-
Notifications
You must be signed in to change notification settings - Fork 40.9k
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
initQueryTimeout and ildeTimeout defaults are not aligned with Cassandra defaults #25150
Conversation
@@ -270,7 +270,7 @@ public Request getRequest() { | |||
* Timeout to use for internal queries that run as part of the initialization | |||
* process, just after a connection is opened. | |||
*/ | |||
private Duration initQueryTimeout = Duration.ofMillis(500); | |||
private Duration initQueryTimeout = Duration.ofSeconds(5); |
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.
@@ -360,7 +360,7 @@ public Throttler getThrottler() { | |||
/** | |||
* Idle timeout before an idle connection is removed. | |||
*/ | |||
private Duration idleTimeout = Duration.ofSeconds(120); | |||
private Duration idleTimeout = Duration.ofSeconds(5); |
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.
That's not what I was expecting. If testing the defaults and fixing the defaults are two separate PRs, including the former in the latter makes the separate PR completely irrelevant. I have reverted. My understanding is that those two properties are tested in the |
Sigh. One of those two is also wrong in |
Sorry for the confusion @snicoll . It passed in 2.3 branch because the value changed in 4.8.1 (used by Boot 24) but was still 500ms in 4.6 driver (used by Boot 23). In hindsight, had I figured that out sooner it would have probably made the most sense to do all of the work in 2.4 rather than 2.3 and partially here. Thanks for your patience. |
See #25130 for background.
@snicoll I went ahead and added the defaults test in this PR as I needed to update it to include the default and also use it to verify the changes.
If however, you want the 2.3.x changes to merge into here (purist tracking standpoint) just lmk and I will remove the test from this PR.