Skip to content

JAVA-2841: Raise timeouts during connection initialization #1480

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

Merged
merged 2 commits into from
Jul 24, 2020
Merged

Conversation

tomekl007
Copy link
Contributor

I've tested the new timeout by connecting to Astra cluster that is in us-west from central Europe using this code:

public class AstraTest {
  public static void main(String[] args) {

    for (int i = 0; i < 100; i++) {
      Path bundle = Paths.get("/Users/tomaszlelek/Downloads/secure-connect-cass-dc.zip");

      try (CqlSession session =
          CqlSession.builder()
              .withCloudSecureConnectBundle(bundle)
              .withAuthCredentials("datastax", "datastax")
              .build()) {
        ResultSet set = session.execute("select * from system.local");
        System.out.println(set.all());
      }
    }
  }
}

with the previous timeout of 500 milliseconds, it was constantly failing to connect.
For the new timeout of 5 seconds, it has successfully connected 100 times.
I don't see a clear reason to increase connect-timeout timeout as well - @adutra can you elaborate more on that suggestion?

@tomekl007 tomekl007 requested review from olim7t and adutra July 22, 2020 10:50
@adutra
Copy link
Contributor

adutra commented Jul 22, 2020

@olim7t knows better than me but afaik connect-timeout is for the duration of the whole connection opening process, whereas init-query-timeout applies to each individual request made during connection initialization. IOW, connect-timeout is necessarily much larger than init-query-timeout since it encompasses many init queries. That's why I figured that setting both timeouts to the same value doesn't make sense.

@olim7t
Copy link
Contributor

olim7t commented Jul 22, 2020

Actually connect-timeout only applies to opening the socket, it does not span protocol negotiation. So I think the current value is fine.

Copy link
Contributor

@olim7t olim7t left a comment

Choose a reason for hiding this comment

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

Don't forget the changelog entry.

nit: could you follow the project's branch naming convention for future PRs? java2841

@@ -255,7 +255,7 @@ protected static void fillWithDriverDefaults(OptionsMap map) {
map.put(TypedDriverOption.LOAD_BALANCING_POLICY_SLOW_AVOIDANCE, true);
map.put(TypedDriverOption.SESSION_LEAK_THRESHOLD, 4);
map.put(TypedDriverOption.CONNECTION_CONNECT_TIMEOUT, Duration.ofSeconds(5));
map.put(TypedDriverOption.CONNECTION_INIT_QUERY_TIMEOUT, Duration.ofMillis(500));
map.put(TypedDriverOption.CONNECTION_INIT_QUERY_TIMEOUT, Duration.ofSeconds(5));
Copy link
Contributor

Choose a reason for hiding this comment

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

reference.conf uses HOCON substitutions to define a few other options in terms of this one. You need to update them too: CONNECTION_SET_KEYSPACE_TIMEOUT, HEARTBEAT_TIMEOUT, CONTROL_CONNECTION_TIMEOUT and REPREPARE_TIMEOUT.

In fact we should probably extract a variable:

    Duration initQueryTimeout = Duration.ofSeconds(5);
    map.put(TypedDriverOption.CONNECTION_INIT_QUERY_TIMEOUT, initQueryTimeout);
    map.put(TypedDriverOption.CONNECTION_SET_KEYSPACE_TIMEOUT, initQueryTimeout);
    ...

Can you also do a pass to handle all other substitutions the same way, and commit that separately?

MapBasedDriverConfigLoaderTest.should_fill_default_profile_like_reference_file will let you know if everything is in sync.

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, what do you mean in a separate commit? Do you want to have two commits in the 4.x after this branch will be merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, because the refactoring to handle other substitutions by extracting a local variable is unrelated to this PR.

Copy link
Contributor

@adutra adutra left a comment

Choose a reason for hiding this comment

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

looks good, but as Olivier suggested, please do a pass to extract local variables for similar cases of hocon references in OptionsMap.

@tomekl007
Copy link
Contributor Author

looks good, but as Olivier suggested, please do a pass to extract local variables for similar cases of hocon references in OptionsMap.

It is already done in:
acc08c0

@adutra
Copy link
Contributor

adutra commented Jul 24, 2020

It is already done in: acc08c0

No, I believe he was asking to apply the same refactoring to other cases of hocon references found in reference.conf.

@tomekl007
Copy link
Contributor Author

It is already done in: acc08c0

No, I believe he was asking to apply the same refactoring to other cases of hocon references found in reference.conf.

You are right, done in 7ad50aa

@tomekl007 tomekl007 requested a review from olim7t July 24, 2020 12:18
@olim7t olim7t merged commit ef161ba into 4.x Jul 24, 2020
@olim7t olim7t deleted the JAVA-2841 branch July 24, 2020 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants