-
Notifications
You must be signed in to change notification settings - Fork 224
Connatix: Enhance endpoint with DC #3878
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| private String getUserId(BidRequest request) { | ||
| final User user = request.getUser(); | ||
| if (user == null) { | ||
| return null; | ||
| } | ||
|
|
||
| final String buyerUid = user.getBuyeruid(); | ||
| if (buyerUid == null || buyerUid.isEmpty()) { | ||
| return null; | ||
| } | ||
|
|
||
| return buyerUid.trim(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private Optional<String> getUserId(BidRequest request) {
return Optional.ofNullable(request.getUser())
.map(User::getBuyeruid)
.filter(StringUtils::isNotBlank)
.map(String::trim);
}| private String getOptimalEndpointUrl(BidRequest request) { | ||
| final String userId = getUserId(request); | ||
| if (userId == null) { | ||
| return endpointUrl; | ||
| } | ||
|
|
||
| final String dataCenterCode = getDataCenterCode(userId); | ||
| if (dataCenterCode == null) { | ||
| return endpointUrl; | ||
| } | ||
|
|
||
| try { | ||
| final URIBuilder uriBuilder = new URIBuilder(endpointUrl); | ||
| return uriBuilder.addParameter("dc", dataCenterCode).build().toString(); | ||
| } catch (URISyntaxException e) { | ||
| throw new PreBidException(e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private String getOptimalEndpointUrl(BidRequest request) {
try {
final URIBuilder uriBuilder = new URIBuilder(endpointUrl);
return getUserId(request)
.map(this::getDataCenterCode)
.map(code -> uriBuilder.addParameter("dc", code))
.orElse(uriBuilder)
.build()
.toString();
} catch (URISyntaxException e) {
throw new PreBidException(e.getMessage());
}
}| errors.add(BidderError.badInput(e.getMessage())); | ||
| return Result.withErrors(errors); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result.withError(...) and create the error list later
|
@AntoxaAntoxic thank you for the review! I applied your suggestions |
AntoxaAntoxic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove all added test JSON files
| return Result.withError(BidderError.badInput("Device IP is required")); | ||
| } | ||
|
|
||
| final List<BidderError> errors = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move creating an error list as closer as possible to the place it's used
| private HttpRequest<BidRequest> makeHttpRequest(BidRequest request, Imp imp, MultiMap headers, | ||
| String optimalEndpointUrl) { | ||
| final BidRequest outgoingRequest = request.toBuilder() | ||
| .imp(List.of(imp)) | ||
| .build(); | ||
|
|
||
| return BidderUtil.defaultRequest(outgoingRequest, headers, endpointUrl, mapper); | ||
| return BidderUtil.defaultRequest(outgoingRequest, headers, optimalEndpointUrl, mapper); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incorrect styling
private HttpRequest<BidRequest> makeHttpRequest(BidRequest request,
Imp imp,
MultiMap headers,
String optimalEndpointUrl) {
final BidRequest outgoingRequest = request.toBuilder().imp(List.of(imp)).build();
return BidderUtil.defaultRequest(outgoingRequest, headers, optimalEndpointUrl, mapper);
}| } | ||
| } | ||
|
|
||
| private Optional<String> getUserId(BidRequest request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static
| .map(String::trim); | ||
| } | ||
|
|
||
| private String getDataCenterCode(String usedId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static
| httpRequests.forEach(request -> { | ||
| assertThat(request.getUri().contains("dc=us-east-2")); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check other occurrences and fix them as well
assertThat(result.getValue).extracting(HttpRequest::getUri).containsOnly("dc=us-east-2");|
@AntoxaAntoxic thanks for the review! all feedback addressed |
| private String getOptimalEndpointUrl(BidRequest request) { | ||
| try { | ||
| final URIBuilder uriBuilder = new URIBuilder(endpointUrl); | ||
| return getUserId(request) | ||
| .map(userId -> getDataCenterCode(userId)) | ||
| .map(dataCenterCode -> uriBuilder.addParameter("dc", dataCenterCode)) | ||
| .orElse(uriBuilder) | ||
| .build() | ||
| .toString(); | ||
| } catch (URISyntaxException e) { | ||
| throw new PreBidException(e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private String getOptimalEndpointUrl(BidRequest request) {
final Optional<String> dataCenterCode = getUserId(request)
.map(ConnatixBidder::getDataCenterCode);
if (dataCenterCode.isEmpty()) {
return endpointUrl;
}
try {
return new URIBuilder(endpointUrl)
.addParameter("dc", dataCenterCode.get())
.build()
.toString();
} catch (URISyntaxException e) {
throw new PreBidException(e.getMessage());
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that @AntoxaAntoxic asked you to change this piece of code. It seems a bit confusing to me (due to .map(<toBuilder>)), let's go back to the old version.
Please make this small change and we will merge the pr as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CTMBNara addressed!
🔧 Type of changes
✨ What's the context?
Addresses #3834
Ports over the following changes from the Go repo prebid/prebid-server#4219
The change consists of appending the Data Center query param to the endpoint url based on the user ID to reduce latency.
🧠 Rationale behind the change
Product requirements from JWPConnatix
🧪 Test plan
Unit tests have been added
🏎 Quality check