-
Notifications
You must be signed in to change notification settings - Fork 118
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
feat: set requestId for fast query path in QueryRequestInfo instead of QueryJobConfiguration #987
Conversation
QueryRequestInfo requestInfo = new QueryRequestInfo(QUERY_JOB_CONFIGURATION_FOR_QUERY); | ||
|
||
when(bigqueryRpcMock.queryRpc(PROJECT, requestInfo.toPb())).thenReturn(queryResponsePb); | ||
when(bigqueryRpcMock.queryRpc(eq(PROJECT), requestPbCapture.capture())) |
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.
Need to validate that what is captured is actually what we expect to see (except the requestId).
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.
Done: 7123195
@@ -49,9 +50,9 @@ | |||
this.maxResults = config.getMaxResults(); | |||
this.query = config.getQuery(); | |||
this.queryParameters = config.toPb().getQuery().getQueryParameters(); | |||
this.requestId = UUID.randomUUID().toString(); |
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.
Are there any cases where a QueryRequestInfo may get reused? Such usage may indicate we set this at time of sending the request (outside of the retry wrapper) rather than time of instantiating the 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.
Yes -- we decided to design it to set this requestId at the time of instantiating the request object which means when a user runs the same QueryJobConfiguration twice, they will be sending two requestIds.
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.
Thanks for clarifying. My worry was mostly around cases where someone may retain a reference and use this in unexpected ways.
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.
Noted -- thank you!
Codecov Report
@@ Coverage Diff @@
## master #987 +/- ##
============================================
- Coverage 80.41% 80.18% -0.23%
+ Complexity 1272 1268 -4
============================================
Files 79 79
Lines 6581 6576 -5
Branches 760 761 +1
============================================
- Hits 5292 5273 -19
- Misses 893 910 +17
+ Partials 396 393 -3
Continue to review full report at Codecov.
|
…f QueryJobConfiguration.
7123195
to
91e1fb9
Compare
As requested @epavan