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.
Need to validate that what is captured is actually what we expect to see (except the requestId).
| 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.
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.
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.
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.
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