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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 6 additions & 6 deletions endpoints/openrtb2/video_auction.go
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,14 +395,14 @@ 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!

}

videoResponse := openrtb_ext.BidResponseVideo{}

// If there were incorrect pods, we put them back to response with error message
if len(podErrors) > 0 {
for _, podEr := range podErrors {
Expand All @@ -412,9 +414,7 @@ func buildVideoResponse(bidresponse *openrtb.BidResponse, podErrors []PodError)
}
}

videoResponse.AdPods = adPods

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

func findAdPod(podInd int64, pods []*openrtb_ext.AdPod) *openrtb_ext.AdPod {
Expand Down
9 changes: 9 additions & 0 deletions endpoints/openrtb2/video_auction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,15 @@ func TestVideoBuildVideoResponsePodErrors(t *testing.T) {
assert.Equal(t, int64(333), bidRespVideo.AdPods[2].PodId, "AdPods should contain error element at index 2")
}

func TestVideoBuildVideoResponseNoBids(t *testing.T) {
openRtbBidResp := openrtb.BidResponse{}
podErrors := make([]PodError, 0, 0)
openRtbBidResp.SeatBid = make([]openrtb.SeatBid, 0)
bidRespVideo, err := buildVideoResponse(&openRtbBidResp, podErrors)
assert.NoError(t, err, "Error should be nil")
assert.Len(t, bidRespVideo.AdPods, 0, "AdPods length should be 0")
}

func mockDeps(t *testing.T, ex *mockExchangeVideo) *endpointDeps {
theMetrics := pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList())
edep := &endpointDeps{
Expand Down
38 changes: 24 additions & 14 deletions exchange/exchange.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,21 +138,26 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque
// Get currency rates conversions for the auction
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())
}
adapterBids, adapterExtra, anyBidsReturned := e.getAllBids(auctionCtx, cleanRequests, aliases, bidAdjustmentFactors, blabels, conversions)

if anyBidsReturned {
bidCategory, adapterBids, err := applyCategoryMapping(ctx, requestExt, adapterBids, *categoriesFetcher, targData)
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...)
auc := newAuction(adapterBids, len(bidRequest.Imp))

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 All @@ -169,11 +174,12 @@ func (e *exchange) makeAuctionContext(ctx context.Context, needsCache bool) (auc
}

// This piece sends all the requests to the bidder adapters and gathers the results.
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) {
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) {
// Set up pointers to the bid results
adapterBids := make(map[openrtb_ext.BidderName]*pbsOrtbSeatBid, len(cleanRequests))
adapterExtra := make(map[openrtb_ext.BidderName]*seatResponseExtra, len(cleanRequests))
chBids := make(chan *bidResponseWrapper, len(cleanRequests))
bidsFound := false

for bidderName, req := range cleanRequests {
// Here we actually call the adapters and collect the bids.
Expand Down Expand Up @@ -228,9 +234,13 @@ func (e *exchange) getAllBids(ctx context.Context, cleanRequests map[openrtb_ext
brw := <-chBids
adapterBids[brw.bidder] = brw.adapterBids
adapterExtra[brw.bidder] = brw.adapterExtra

if !bidsFound && adapterBids[brw.bidder] != nil && len(adapterBids[brw.bidder].bids) > 0 {
bidsFound = true
}
}

return adapterBids, adapterExtra
return adapterBids, adapterExtra, bidsFound
}

func (e *exchange) recoverSafely(inner func(openrtb_ext.BidderName, openrtb_ext.BidderName, *openrtb.BidRequest, *pbsmetrics.AdapterLabels, currencies.Conversions), chBids chan *bidResponseWrapper) func(openrtb_ext.BidderName, openrtb_ext.BidderName, *openrtb.BidRequest, *pbsmetrics.AdapterLabels, currencies.Conversions) {
Expand Down