-
Notifications
You must be signed in to change notification settings - Fork 85
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
fix: fix deadlock issue in ConnectionWorkerPool #1938
Conversation
prerequisite for multiplexing client
new stream name as a switch of destinationt
also fixed a tiny bug inside fake bigquery write impl for getting thre response from offset
possible the proto schema does not contain this field
...uerystorage/src/test/java/com/google/cloud/bigquery/storage/v1/ConnectionWorkerPoolTest.java
Outdated
Show resolved
Hide resolved
Instant insertTime = requestWrapper.insertTime; | ||
Instant currentTime = Instant.now(); | ||
long latencyToAdd = currentTime.toEpochMilli() - insertTime.toEpochMilli(); | ||
updateLatencyDistribution(latencyToAdd); |
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.
Let's add the error message for now from client lib at L686. So in case there is still a blockage on customer's case, we will have more info to investigate.
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.
Do you mean the error message in response? I think that's already printed by Pablo's current code... I was planning to revert the change in this file once Pablo copy over this part
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 want it to be in the release we hand over to customers, so if anything is going on, we have some logs.
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.
let me clean this file up, Pablo has already copied the stats log, let's add error log directly
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.
But won't that kind of log overwhelmed the log console?
this.reportingThread.start(); | ||
} | ||
|
||
private void reportSingleCurrentStatus() { |
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.
Is this too much logging? Is it going to be in the final code?
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.
Not going to be in the final code, it's reported every 5 seconds
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:
Fixes #<issue_number_goes_here> ☕️
If you write sample code, please follow the samples format.