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

feat: update StreamWriterV2 to support trace id #895

Merged
merged 8 commits into from Mar 2, 2021
Merged

Conversation

yayi-google
Copy link
Contributor

@yayi-google yayi-google commented Feb 26, 2021

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

@yayi-google yayi-google requested review from yirutang and bigang-li Feb 26, 2021
@yayi-google yayi-google requested a review from as a code owner Feb 26, 2021
@product-auto-label product-auto-label bot added the api: bigquerystorage label Feb 26, 2021
@google-cla google-cla bot added the cla: yes label Feb 26, 2021
@yayi-google yayi-google removed the request for review from bigang-li Feb 26, 2021
@codecov
Copy link

@codecov codecov bot commented Feb 26, 2021

Codecov Report

Merging #895 (0db5ef9) into master (3b60f44) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #895      +/-   ##
============================================
+ Coverage     80.57%   80.63%   +0.06%     
- Complexity     1015     1016       +1     
============================================
  Files            76       76              
  Lines          5533     5542       +9     
  Branches        427      429       +2     
============================================
+ Hits           4458     4469      +11     
+ Misses          897      896       -1     
+ Partials        178      177       -1     
Impacted Files Coverage Δ Complexity Δ
...cloud/bigquery/storage/v1beta2/StreamWriterV2.java 96.10% <100.00%> (+0.15%) 39.00 <0.00> (+1.00)
.../cloud/bigquery/storage/v1alpha2/StreamWriter.java 80.39% <0.00%> (+0.43%) 34.00% <0.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 3b60f44...5ca7a28. Read the comment docs.

/** TraceId for debuging purpose. */
public Builder setTraceId(String traceId) {
this.traceId = traceId;
return this;
Copy link
Collaborator

@bigang-li bigang-li Feb 26, 2021

Choose a reason for hiding this comment

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

Let's perform some simple sanity check here.

On the server side, this needs to be something like key value pair like http header or A:B; is okay as well (we need to document it).

A simple processing at the client side will be splitting the trace id per the standard above if we can;
other wise convert it to UserTrace:xxx;

On top of it, adding the StreamWrite:Version?

Copy link
Contributor

@yirutang yirutang Feb 26, 2021

Choose a reason for hiding this comment

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

The traceId name is strictly screened in the backend. In order for dataflow to be recognized, it has to start from Dataflow:, so apart from this function, maybe also provide a setDataflowJobId to set it directly to Dataflow:job_id?

Copy link
Contributor Author

@yayi-google yayi-google Feb 26, 2021

Choose a reason for hiding this comment

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

All these requires backend support, which is not there yet.

I would also prefer to put all the logic you mentioned into the server side, because we have strong control of the codes there.

On the other hand, it is very hard to change the client library. So a free string seems most flexible.

Copy link
Contributor

@yirutang yirutang Feb 26, 2021

Choose a reason for hiding this comment

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

Making dataflow do the right thing is also fine to me. Just that if they mess up, then we will have issues in debugging their traffic. The backend already parses these trace ids.

Copy link
Collaborator

@bigang-li bigang-li Feb 27, 2021

Choose a reason for hiding this comment

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

Yes, we need to perform such kind of logic in the server side, but here, let's perform some sanity check and makes sure that final trace id is in a good shape as well?

Copy link
Contributor Author

@yayi-google yayi-google Mar 2, 2021

Choose a reason for hiding this comment

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

Done. Added check to make sure traceId follow the format of A:B.

@stephaniewang526
Copy link
Member

@stephaniewang526 stephaniewang526 commented Feb 26, 2021

Can you take a look at this?

StreamWriterTest.testFlushAllFailed:893 Unexpected exception:java.lang.InterruptedException: Wait timeout

I have seen this happen a few times now. I worry it's a flaky test.
Full trace

@yayi-google
Copy link
Contributor Author

@yayi-google yayi-google commented Feb 26, 2021

Can you take a look at this?

StreamWriterTest.testFlushAllFailed:893 Unexpected exception:java.lang.InterruptedException: Wait timeout

I have seen this happen a few times now. I worry it's a flaky test.
Full trace

Taken care in a separate PR.

@stephaniewang526
Copy link
Member

@stephaniewang526 stephaniewang526 commented Mar 1, 2021

Looks like there are merge conflicts -- PTAL.

@yayi-google
Copy link
Contributor Author

@yayi-google yayi-google commented Mar 2, 2021

Looks like there are merge conflicts -- PTAL.

Done. Fixed the conflicts.

@stephaniewang526 stephaniewang526 merged commit 2e49ce8 into master Mar 2, 2021
16 checks passed
@stephaniewang526 stephaniewang526 deleted the trace-id branch Mar 2, 2021
gcf-merge-on-green bot pushed a commit that referenced this issue Mar 4, 2021
🤖 I have created a release \*beep\* \*boop\*
---
## [1.14.0](https://www.github.com/googleapis/java-bigquerystorage/compare/v1.13.0...v1.14.0) (2021-03-04)


### Features

* update StreamWriterV2 to support trace id ([#895](https://www.github.com/googleapis/java-bigquerystorage/issues/895)) ([2e49ce8](https://www.github.com/googleapis/java-bigquerystorage/commit/2e49ce8c79cb059840c3307898ba16980f6892fa))


### Bug Fixes

* add schema update back to json writer ([#905](https://www.github.com/googleapis/java-bigquerystorage/issues/905)) ([a2adbf8](https://www.github.com/googleapis/java-bigquerystorage/commit/a2adbf80753161cbddd23d5a7db75e9250db58fa))
* Add unit test for concurrent issues we worried about, and fix some locking issues ([#854](https://www.github.com/googleapis/java-bigquerystorage/issues/854)) ([0870797](https://www.github.com/googleapis/java-bigquerystorage/commit/087079728195e20f93701e8d5e1e59ba29a7d21b))
* test failure testAppendWhileShutdownSuccess ([#904](https://www.github.com/googleapis/java-bigquerystorage/issues/904)) ([b80183e](https://www.github.com/googleapis/java-bigquerystorage/commit/b80183ea23c8b78611a42d22d8c62a4ba4904a80))
* testAppendWhileShutdownSuccess race ([#907](https://www.github.com/googleapis/java-bigquerystorage/issues/907)) ([d39443d](https://www.github.com/googleapis/java-bigquerystorage/commit/d39443d51d2625e4b3aee59d1e593229e9e449d3))


### Dependencies

* update dependency com.google.cloud:google-cloud-bigquery to v1.127.6 ([#909](https://www.github.com/googleapis/java-bigquerystorage/issues/909)) ([505938b](https://www.github.com/googleapis/java-bigquerystorage/commit/505938bcba5a4a7af9e618572bbc41f365702f47))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquerystorage cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants