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

Jump to bottom

feat: set requestId for fast query path in QueryRequestInfo instead of QueryJobConfiguration #987

Merged
merged 6 commits into from Dec 5, 2020
Merged

feat: set requestId for fast query path in QueryRequestInfo instead of QueryJobConfiguration #987

merged 6 commits into from Dec 5, 2020

Conversation

stephaniewang526
Copy link
Contributor

@stephaniewang526 stephaniewang526 commented Dec 2, 2020 •
edited

As requested @epavan

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 2, 2020
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/java-bigquery API. label Dec 2, 2020
@stephaniewang526 stephaniewang526 assigned pmakani and unassigned pmakani Dec 2, 2020
@stephaniewang526 stephaniewang526 marked this pull request as ready for review December 4, 2020 23:26
@stephaniewang526 stephaniewang526 requested a review from a team December 4, 2020 23:26
@stephaniewang526 stephaniewang526 requested a review from a team as a code owner December 4, 2020 23:26
epavan123
epavan123 reviewed Dec 4, 2020
View reviewed changes
google-cloud-bigquery/src/test/java/com/google/cloud/bigquery/BigQueryImplTest.java
QueryRequestInfo requestInfo = new QueryRequestInfo(QUERY_JOB_CONFIGURATION_FOR_QUERY);

when(bigqueryRpcMock.queryRpc(PROJECT, requestInfo.toPb())).thenReturn(queryResponsePb);
when(bigqueryRpcMock.queryRpc(eq(PROJECT), requestPbCapture.capture()))
Copy link

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).

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: 7123195

shollyman
shollyman reviewed Dec 4, 2020
View reviewed changes
google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/QueryRequestInfo.java
@@ -49,9 +50,9 @@
this.maxResults = config.getMaxResults();
this.query = config.getQuery();
this.queryParameters = config.toPb().getQuery().getQueryParameters();
this.requestId = UUID.randomUUID().toString();
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted -- thank you!

@codecov
Copy link

codecov bot commented Dec 4, 2020 •
edited

Codecov Report

Merging #987 (7123195) into master (120a48d) will decrease coverage by 0.22%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             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
Impacted Files Coverage Δ Complexity Δ
...m/google/cloud/bigquery/QueryJobConfiguration.java 90.21% <100.00%> (-0.14%) 67.00 <0.00> (-1.00)
...va/com/google/cloud/bigquery/QueryRequestInfo.java 52.23% <100.00%> (-20.90%) 10.00 <0.00> (-3.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 120a48d...91e1fb9. Read the comment docs.

@stephaniewang526 stephaniewang526 changed the title fix: set requestId for fast query path in QueryRequestInfo instead of QueryJobConfiguration feat: set requestId for fast query path in QueryRequestInfo instead of QueryJobConfiguration Dec 5, 2020
@stephaniewang526 stephaniewang526 added the automerge Merge the pull request once unit tests and other checks pass. label Dec 5, 2020
@gcf-merge-on-green gcf-merge-on-green bot merged commit e2cd4f6 into master Dec 5, 2020
@gcf-merge-on-green gcf-merge-on-green bot deleted the refactor-requestId branch December 5, 2020 00:42
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shollyman shollyman shollyman approved these changes

@epavan123 epavan123 epavan123 approved these changes

@pmakani pmakani Awaiting requested review from pmakani

Assignees
No one assigned
Labels
api: bigquery Issues related to the googleapis/java-bigquery API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@stephaniewang526 @shollyman @epavan123 @pmakani