KEMBAR78
feat: add timeout to inflight queue waiting by GaoleMeng · Pull Request #1957 · googleapis/java-bigquerystorage · GitHub
Skip to content

Conversation

@GaoleMeng
Copy link
Contributor

If we don't see the exception thrown by timeout on waiting, we then know it's not deadlock on waiting for in flight queue to have space

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> ☕️

If you write sample code, please follow the samples format.

GaoleMeng and others added 30 commits September 13, 2022 01:58
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
@GaoleMeng GaoleMeng requested review from a team and rosiezou January 25, 2023 10:30
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: bigquerystorage Issues related to the googleapis/java-bigquerystorage API. labels Jan 25, 2023
@GaoleMeng GaoleMeng requested a review from yirutang January 25, 2023 10:44
}

try {
log.info("Begin shutting down user callback thread pool for stream " + streamName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this logging accurate? with multiplexing, the identifier shouldn't be streamName. It is not just for this change but in general, seems streamName is not good enough to indicate a connection.

Also seems we should change this log to fine level.

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 need a new kind of identifier besides stream name

let's change them together in the next PR

changed to fine

+ " is interrupted with exception: "
+ e.toString());
}
log.info("User close finishes for stream " + streamName);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove the line on 395?

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

try {
log.info("Begin shutting down user callback thread pool for stream " + streamName);
threadPool.shutdown();
threadPool.awaitTermination(3, TimeUnit.MINUTES);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log when we actually timed out here?

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

private static final Logger log = Logger.getLogger(StreamWriter.class.getName());

// Maximum wait time on inflight quota before error out.
private static long INFLIGHT_QUOTA_MAX_WAIT_TIME_MILLI = 180000;
Copy link
Contributor

Choose a reason for hiding this comment

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

Make the default to 5 minutes?

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

@GaoleMeng GaoleMeng added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 25, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 25, 2023
@GaoleMeng GaoleMeng added the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 25, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 25, 2023
@GaoleMeng GaoleMeng merged commit 3159b12 into googleapis:main Jan 25, 2023
gcf-merge-on-green bot pushed a commit that referenced this pull request Feb 1, 2023
🤖 I have created a release *beep* *boop*
---


## [2.29.0](https://togithub.com/googleapis/java-bigquerystorage/compare/v2.28.4...v2.29.0) (2023-02-01)


### Features

* Add timeout to inflight queue waiting ([#1957](https://togithub.com/googleapis/java-bigquerystorage/issues/1957)) ([3159b12](https://togithub.com/googleapis/java-bigquerystorage/commit/3159b120e5cd388cf9776a1fa928a3e6ae105d9d))
* Allow java client to handle schema change during same stream name  ([#1964](https://togithub.com/googleapis/java-bigquerystorage/issues/1964)) ([305f71e](https://togithub.com/googleapis/java-bigquerystorage/commit/305f71ee4b274df58388fc3000e9f5da9fc908e1))


### Bug Fixes

* At connection level, retry for internal errors ([#1965](https://togithub.com/googleapis/java-bigquerystorage/issues/1965)) ([9c01bc1](https://togithub.com/googleapis/java-bigquerystorage/commit/9c01bc11b51dc1e3e209e4d6b666b9ddd3212cf5))
* Reduce visibility of the ConnectionPool and ConnectionWorker, so… ([#1954](https://togithub.com/googleapis/java-bigquerystorage/issues/1954)) ([dcb234b](https://togithub.com/googleapis/java-bigquerystorage/commit/dcb234b95d0812d4d91b0c206d0b7e0fb30ab0fa))
* Remove unrecoverable connection from connection pool during multiplexing  ([#1967](https://togithub.com/googleapis/java-bigquerystorage/issues/1967)) ([091dddb](https://togithub.com/googleapis/java-bigquerystorage/commit/091dddb9b2baf1f4b481e8d7961d451b71a8508b))

---
This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.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 Issues related to the googleapis/java-bigquerystorage API. size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants