Skip to content
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.

Sign up for GitHub

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

Cache validation fix #911

Merged
merged 6 commits into from
Jun 5, 2019
Merged

Cache validation fix #911

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0b83c27
Cache validation fix
May 21, 2019
3e84ad8
Cache validation fix
May 21, 2019
043be36
Cache validation fix: minor refactoring
May 29, 2019
6739537
Cache validation fix: minor refactoring
May 31, 2019
e82b10e
Cache validation fix: added unit test for no bids returned
Jun 1, 2019
abff28a
Cache validation fix: minor refactoring
Jun 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion endpoints/openrtb2/video_auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,8 +359,10 @@ func minMax(array []int) (int, int) {
func buildVideoResponse(bidresponse *openrtb.BidResponse, podErrors []PodError) (*openrtb_ext.BidResponseVideo, error) {

adPods := make([]*openrtb_ext.AdPod, 0)
anyBidsReturned := false
for _, seatBid := range bidresponse.SeatBid {
for _, bid := range seatBid.Bid {
anyBidsReturned = true

var tempRespBidExt openrtb_ext.ExtBid
if err := json.Unmarshal(bid.Ext, &tempRespBidExt); err != nil {
Expand Down Expand Up @@ -393,7 +395,9 @@ func buildVideoResponse(bidresponse *openrtb.BidResponse, podErrors []PodError)
}
}

if len(adPods) == 0 {
//check if there are any bids in response.
//if there are no bids - empty response should be returned, no cache errors
if len(adPods) == 0 && anyBidsReturned {
//means there is a global cache error, we need to reject all bids
err := errors.New("caching failed for all bids")
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kalypsonika this is very, very minor but, just for the sake of saving memory in the heap and trying to declare as less local variables as possible, can we get rid of lines 406 and 419 and change the return statement in line 421?

	return &openrtb_ext.BidResponseVideo{AdPods:adPods}, nil

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Expand Down
30 changes: 20 additions & 10 deletions exchange/exchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,20 +139,30 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque
conversions := e.currencyConverter.Rates()

adapterBids, adapterExtra := e.getAllBids(auctionCtx, cleanRequests, aliases, bidAdjustmentFactors, blabels, conversions)
bidCategory, adapterBids, err := applyCategoryMapping(ctx, requestExt, adapterBids, *categoriesFetcher, targData)
auc := newAuction(adapterBids, len(bidRequest.Imp))
if err != nil {
return nil, fmt.Errorf("Error in category mapping : %s", err.Error())
anyBidsReturned := false
for _, seatBid := range adapterBids {
if seatBid != nil && len(seatBid.bids) > 0 {
anyBidsReturned = true
break
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can save a bit of processing time if we remove the for statement in line 143 and make the getAllBids() method to return the anyBidsReturned value. Since the adapterBids map gets already traversed inside getAllBids(), we could save some processing time by not going through the rather large adapterBids map again and make our solution a bit less verbose.
In other words, go from this:

141 adapterBids, adapterExtra := e.getAllBids(auctionCtx, cleanRequests, aliases, bidAdjustmentFactors, blabels, conversions)
142 anyBidsReturned := false
143 for _, seatBid := range adapterBids {
144 	if seatBid != nil && len(seatBid.bids) > 0 {
145 		anyBidsReturned = true
146 		break
147 	}
148 }
149 if anyBidsReturned {

To something like this:

141 adapterBids, adapterExtra, anyBidsReturned := e.getAllBids(auctionCtx, cleanRequests, aliases, bidAdjustmentFactors, blabels, conversions)
142 if anyBidsReturned {

Inside of getAllBids() we can probably populate the boolean value in the last if statement:
Go from:

183 func (e *exchange) getAllBids(ctx context.Context, cleanRequests map[openrtb_ext.BidderName]*openrtb.BidRequest, aliases map[string]string, bidAdjustments map[string]float64, blabels map[openrtb_ext.BidderName]*pbsmetrics.AdapterLabels, conversions currencies.Conversions) (map[openrtb_ext.BidderName]*pbsOrtbSeatBid, map[openrtb_ext.BidderName]*seatResponseExtra) {
184     // Set up pointers to the bid results
185     adapterBids := make(map[openrtb_ext.BidderName]*pbsOrtbSeatBid, len(cleanRequests))
186     adapterExtra := make(map[openrtb_ext.BidderName]*seatResponseExtra, len(cleanRequests))
187     chBids := make(chan *bidResponseWrapper, len(cleanRequests))
188
189     for bidderName, req := range cleanRequests {
190         // Here we actually call the adapters and collect the bids.
191 +-- 45 lines: coreBidder := resolveBidder(string(bidderName), aliases)--------------------------------------------
236     }
237     // Wait for the bidders to do their thing
238     for i := 0; i < len(cleanRequests); i++ {
239         brw := <-chBids
240         adapterBids[brw.bidder] = brw.adapterBids
241         adapterExtra[brw.bidder] = brw.adapterExtra
242     }
243
244     return adapterBids, adapterExtra
245 }

To something like:

176 func (e *exchange) getAllBids(ctx context.Context, cleanRequests map[openrtb_ext.BidderName]*openrtb.BidRequest, aliases map[string]string, bidAdjustments map[string]float64, blabels map[openrtb_ext.BidderName]*pbsmetrics.AdapterLabels, conversions currencies.Conversions) (map[openrtb_ext.BidderName]*pbsOrtbSeatBid, map[openrtb_ext.BidderName]*seatResponseExtra, bool) {
177     // Set up pointers to the bid results
178     adapterBids := make(map[openrtb_ext.BidderName]*pbsOrtbSeatBid, len(cleanRequests))
179     adapterExtra := make(map[openrtb_ext.BidderName]*seatResponseExtra, len(cleanRequests))
180     bidsFound := false  // NEW
181     chBids := make(chan *bidResponseWrapper, len(cleanRequests))
182
183     for bidderName, req := range cleanRequests {
184         // Here we actually call the adapters and collect the bids.
185 +-- 45 lines: coreBidder := resolveBidder(string(bidderName), aliases)---------------------------------
230     }
231     // Wait for the bidders to do their thing
232     for range cleanRequests {
233         //for i := 0; i < len(cleanRequests); i++ {
234         brw := <-chBids
235         adapterBids[brw.bidder] = brw.adapterBids
236         if adapterBids[brw.bidder] != nil && len(adapterBids[brw.bidder].bids) > 0 { // NEW
237             bidsFound = true        // NEW
238         }                           // NEW
239         adapterExtra[brw.bidder] = brw.adapterExtra
240     }
241
242     return adapterBids, adapterExtra, bidsFound
243 }

What do you think? Please let me know if you agree with any of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good idea! Refactored.

if anyBidsReturned {
bidCategory, adapterBids, err := applyCategoryMapping(ctx, requestExt, adapterBids, *categoriesFetcher, targData)
auc := newAuction(adapterBids, len(bidRequest.Imp))
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this err be checked before the previous line: auc := newAuction(adapterBids, len(bidRequest.Imp))? I understand that this PR isn't the one introducing this change and it was like that before but this seems like a small change that would be good to have because we're calling newAuction unnecessarily in case of an err from applyCategoryMapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally makes sense. Done.

return nil, fmt.Errorf("Error in category mapping : %s", err.Error())
}

if targData != nil && adapterBids != nil {
auc.setRoundedPrices(targData.priceGranularity)
cacheErrs := auc.doCache(ctx, e.cache, targData, bidRequest, 60, &e.defaultTTLs, bidCategory)
if len(cacheErrs) > 0 {
errs = append(errs, cacheErrs...)
if targData != nil {
auc.setRoundedPrices(targData.priceGranularity)
cacheErrs := auc.doCache(ctx, e.cache, targData, bidRequest, 60, &e.defaultTTLs, bidCategory)
if len(cacheErrs) > 0 {
errs = append(errs, cacheErrs...)
}
targData.setTargeting(auc, bidRequest.App != nil, bidCategory)
}
targData.setTargeting(auc, bidRequest.App != nil, bidCategory)
}

// Build the response
return e.buildBidResponse(ctx, liveAdapters, adapterBids, bidRequest, resolvedRequest, adapterExtra, errs)
}
Expand Down