-
Notifications
You must be signed in to change notification settings - Fork 732
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
Use Concurrency To Download Vendor List #4005
base: master
Are you sure you want to change the base?
Changes from all commits
0c48c70
e0b1be2
0a5b937
7623704
e2baac5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import ( | |
"net/http" | ||
"net/http/httptest" | ||
"strconv" | ||
"sync" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
|
@@ -192,11 +193,15 @@ type versionInfo struct { | |
} | ||
type saver []versionInfo | ||
|
||
var saverMutex sync.Mutex | ||
|
||
func (s *saver) saveVendorLists(specVersion uint16, listVersion uint16, gvl api.VendorList) { | ||
vi := versionInfo{ | ||
specVersion: specVersion, | ||
listVersion: listVersion, | ||
} | ||
saverMutex.Lock() | ||
defer saverMutex.Unlock() | ||
*s = append(*s, vi) | ||
} | ||
|
||
|
@@ -253,7 +258,10 @@ func TestPreloadCache(t *testing.T) { | |
defer server.Close() | ||
|
||
s := make(saver, 0, 5) | ||
preloadCache(context.Background(), server.Client(), testURLMaker(server), s.saveVendorLists) | ||
preloadCache(context.Background(), server.Client(), testURLMaker(server), s.saveVendorLists, config.VendorListFetcher{ | ||
MaxConcurrencyInitFetchLatestVersion: 2, | ||
MaxConcurrencyInitFetchSpecificVersion: 2, | ||
}) | ||
|
||
expectedLoadedVersions := []versionInfo{ | ||
{specVersion: 2, listVersion: 2}, | ||
|
@@ -284,12 +292,6 @@ var vendorList2Expected = testExpected{ | |
vendorPurposes: map[int]bool{1: false, 2: true, 3: true}, | ||
} | ||
|
||
var vendorListFallbackExpected = testExpected{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this code block is not used at all so i deleted it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you need to delete it to accomplish your goal in this PR ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but why we need to keep it if the code is not used at all? there is no comment or description associated with the deleted test case. i used a practice called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. I see no problem with that. |
||
vendorListVersion: 215, // Values from hardcoded fallback file. | ||
vendorID: 12, | ||
vendorPurposes: map[int]bool{1: true, 2: false, 3: true}, | ||
} | ||
|
||
type vendorList struct { | ||
GVLSpecificationVersion uint16 `json:"gvlSpecificationVersion"` | ||
VendorListVersion uint16 `json:"vendorListVersion"` | ||
|
@@ -409,5 +411,9 @@ func testConfig() config.GDPR { | |
InitVendorlistFetch: 60 * 1000, | ||
ActiveVendorlistFetch: 1000 * 5, | ||
}, | ||
VendorListFetcher: config.VendorListFetcher{ | ||
MaxConcurrencyInitFetchLatestVersion: 2, | ||
MaxConcurrencyInitFetchSpecificVersion: 2, | ||
}, | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,13 @@ status_response: "ok" # default response string for /status endpoint | |
|
||
gdpr: | ||
default_value: "0" # disable gdpr, explicitly specifying a default value is a requirement in prebid server config | ||
vendorlist_fetcher: | ||
max_concurrency_init_fetch_latest_version: 2 | ||
max_concurrency_init_fetch_specific_version: 20 | ||
timeouts_ms: | ||
init_vendorlist_fetches: 30000 | ||
active_vendorlist_fetch: 30000 | ||
Comment on lines
+9
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are those new configuration keys optional ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @zhongshixi I am not asking what are those new configuration keys for. I am asking whether or not the Prebid Server software works if you don't provide them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebhtml
we have default settings, which is literally the same as before ( sequential download ) for
let me know if i answered your question There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it's not backward-compatible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebhtml i am not following
in summary, i do not think this PR incurs any compatibility issue. So i would like to ask if could you point out exactly which part of the code logic is not backwards compatible, maybe i am actually missing something |
||
|
||
|
||
# set up stored request storage using local file system | ||
stored_requests: | ||
|
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 think that the vertical alignment was intentional.
Why change the indentation style of those lines ?
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.
@sebhtml do you mind elaborating more about
vertical alignment was intentional
? Changing the indentation is automatically done by the go formatter ( i am using MS Visual Studio ) , which i believe it is easier to read.but i am curious about other technical aspects related to it
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.
The types are vertically aligned:
see
prebid-server/config/config.go
Line 258 in 0a5b937
I don't know if my comment is relevant though.
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.
It may be go fmt's decision; they actually look wrong before.
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.
@bsardo let me know your opinion