Skip to content

feat: round robin host selection strategy #603

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 12, 2023
Merged

feat: round robin host selection strategy #603

merged 2 commits into from
Sep 12, 2023

Conversation

crystall-bitquill
Copy link
Contributor

Summary

Round robin host selection strategy

Description

  • added the round robin host selection strategy
  • moved the least connections strategy to a host selector class

Additional Reviewers

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@crystall-bitquill crystall-bitquill force-pushed the rw-splitting-round-robin branch from 6f74c08 to 046fa2b Compare August 16, 2023 20:33
brunos-bq
brunos-bq reviewed Aug 17, 2023
View reviewed changes
wrapper/src/main/java/software/amazon/jdbc/HikariPooledConnectionProvider.java Outdated
"ConnectionProvider.unsupportedHostSpecSelectorStrategy",
new Object[] {strategy, HikariPooledConnectionProvider.class}));
switch (strategy) {
case "leastConnections":
Copy link
Contributor

Choose a reason for hiding this comment

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

strings

@crystall-bitquill crystall-bitquill force-pushed the rw-splitting-round-robin branch 3 times, most recently from 3847b5c to a241109 Compare August 21, 2023 19:40
@crystall-bitquill crystall-bitquill force-pushed the rw-splitting-round-robin branch 3 times, most recently from 6d9cb35 to bffdcce Compare August 24, 2023 19:22
karenc-bq
karenc-bq reviewed Aug 25, 2023
View reviewed changes
@crystall-bitquill crystall-bitquill force-pushed the rw-splitting-round-robin branch 2 times, most recently from 5f91613 to 614592f Compare August 26, 2023 08:48
sergiyvamz
sergiyvamz reviewed Aug 29, 2023
View reviewed changes
@crystall-bitquill crystall-bitquill force-pushed the rw-splitting-round-robin branch 3 times, most recently from 5acfb32 to 9911abb Compare September 1, 2023 21:39
sergiyvamz
sergiyvamz reviewed Sep 7, 2023
View reviewed changes
wrapper/src/main/java/software/amazon/jdbc/HikariPooledConnectionProvider.java Outdated
@@ -98,6 +103,8 @@ public HikariPooledConnectionProvider(
HikariPoolConfigurator hikariPoolConfigurator, HikariPoolMapping mapping) {
this.poolConfigurator = hikariPoolConfigurator;
this.poolMapping = mapping;
final LeastConnectionsHostSelector hostSelector = new LeastConnectionsHostSelector(databasePools);
acceptedStrategies.put(LeastConnectionsHostSelector.STRATEGY_LEAST_CONNECTIONS, hostSelector);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can't add this strategy to a static map. The strategy belongs to a particular instance of this connection provider and uses it resources (databasePools). If you add it to a static collection, other connection providers may use it and it's bad.

sergiyvamz
sergiyvamz reviewed Sep 7, 2023
View reviewed changes
wrapper/src/main/java/software/amazon/jdbc/HikariPooledConnectionProvider.java Outdated
@@ -145,44 +154,21 @@ public boolean acceptsUrl(

@Override
public boolean acceptsStrategy(@NonNull HostRole role, @NonNull String strategy) {
return LEAST_CONNECTIONS_STRATEGY.equals(strategy);
return acceptedStrategies.containsKey(strategy);
Copy link
Contributor

Choose a reason for hiding this comment

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

return acceptedStrategies.containsKey(strategy) || LEAST_CONNECTIONS_STRATEGY.equals(strategy);

sergiyvamz
sergiyvamz reviewed Sep 7, 2023
View reviewed changes
wrapper/src/main/java/software/amazon/jdbc/HikariPooledConnectionProvider.java Outdated
throws SQLException {
if (!LEAST_CONNECTIONS_STRATEGY.equals(strategy)) {
if (!acceptedStrategies.containsKey(strategy)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to previous comment.

sergiyvamz
sergiyvamz reviewed Sep 7, 2023
View reviewed changes
wrapper/src/main/java/software/amazon/jdbc/HostSelector.java Outdated
* @return a host matching the requested role
* @throws SQLException if the host list does not contain any hosts matching the requested role or
* an error occurs while selecting a host
*/
HostSpec getHost(List<HostSpec> hosts, HostRole role) throws SQLException;
HostSpec getHost(List<HostSpec> hosts, HostRole role, Properties props) throws SQLException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we annotate props with @nullable? It seems that some implementation may not need properties.

sergiyvamz
sergiyvamz reviewed Sep 7, 2023
View reviewed changes
wrapper/src/main/java/software/amazon/jdbc/RoundRobinHostSelector.java Outdated
return eligibleHosts.get(targetHostIndex);
}

private synchronized void createCacheEntryForHosts(
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is private and is used in getHost() that is public and synchronized. You can safely remove "synchronized" here.

sergiyvamz
sergiyvamz reviewed Sep 7, 2023
View reviewed changes
wrapper/src/main/java/software/amazon/jdbc/RoundRobinHostSelector.java Outdated
}
}

private synchronized void updateCachePropertiesForRoundRobinClusterInfo(
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar note about synchronized

@crystall-bitquill crystall-bitquill force-pushed the rw-splitting-round-robin branch 3 times, most recently from 9e2c3b4 to f403c72 Compare September 8, 2023 22:05
@karenc-bq karenc-bq enabled auto-merge (squash) September 12, 2023 21:47
@karenc-bq karenc-bq merged commit 6754855 into aws:main Sep 12, 2023
@crystall-bitquill crystall-bitquill deleted the rw-splitting-round-robin branch November 15, 2023 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karenc-bq karenc-bq karenc-bq left review comments

@aaronchung-bitquill aaronchung-bitquill aaronchung-bitquill left review comments

@sergiyvamz sergiyvamz sergiyvamz approved these changes

+1 more reviewer

@brunos-bq brunos-bq brunos-bq left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@crystall-bitquill @brunos-bq @karenc-bq @sergiyvamz @aaronchung-bitquill