feat: implement autoscaling and the leastConnections strategy #451

congoamz · 2023-05-17T16:57:59Z

Summary

Description

Additional Reviewers

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

implement computeIfAbsent() for CacheMap; provide dispose callback function to properly close DataSource;

sergiyvamz · 2023-05-18T17:52:05Z

wrapper/src/main/java/software/amazon/jdbc/util/CacheMap.java

+  }
+
+  public CacheMap(
+      final IsItemValidFunc<V> isItemValidFunc,


canDisposeFunc ?

sergiyvamz · 2023-05-18T17:54:00Z

wrapper/src/main/java/software/amazon/jdbc/util/CacheMap.java


 public class CacheMap<K, V> {

-  private final Map<K, CacheItem<V>> cache = new ConcurrentHashMap<>();
-  private final long cleanupIntervalNanos = TimeUnit.MINUTES.toNanos(10);
+  private final Map<K, CacheItem> cache = new ConcurrentHashMap<>();


What's the reason to remove V?

sergiyvamz · 2023-05-18T18:23:13Z

wrapper/src/main/java/software/amazon/jdbc/util/CacheMap.java


    public CacheItem(final V item, final long expirationTime) {
      this.item = item;
      this.expirationTime = expirationTime;
    }

    boolean isExpired() {
+      if (isItemValidFunc != null) {
+        return System.nanoTime() > expirationTime && !isItemValidFunc.isValid(this.item);


Hmm I'm not sure what's the meaning of isItemValidFunc. Need to discuss.

sergiyvamz · 2023-05-18T18:27:37Z

wrapper/src/test/java/integration/refactored/TestInstanceInfo.java

@@ -23,32 +23,36 @@
 public class TestInstanceInfo {

  private String instanceId; // "instance-1"
-  private String endpoint; // "instance-1.ABC.cluster-XYZ.us-west-2.rds.amazonaws.com"
-  private int endpointPort;
+  private String host; // "instance-1.ABC.cluster-XYZ.us-west-2.rds.amazonaws.com"


This change is probably unrelated to leastConnections strategy. I'd make it in a separate PR if needed. We have quite stable integration suite and renaming of properties is definitely can do but it should be a strong reason for doing that.

sergiyvamz · 2023-05-18T18:29:52Z

wrapper/src/test/java/integration/refactored/container/tests/AutoscalingTests.java

+            .anyMatch((url) -> url.equals(newInstance.getUrl())));
+        newInstanceConn.setReadOnly(false);
+      } finally {
+        auroraUtil.deleteInstance(newInstance);


What's the consequences for other integrations tests? We probably should run autoscaling tests in a separate run (like we do for hibernate tests) so changes in cluster configuration makes no side effects on other tests.

sergiyvamz · 2023-05-18T18:33:25Z

wrapper/src/test/java/integration/refactored/container/tests/ReadWriteSplittingTests.java

+      final HikariConfig config = new HikariConfig();
+      config.setMaximumPoolSize(maxPoolSize);
+      config.setInitializationFailTimeout(75000);
+      config.setKeepaliveTime(30000);


TCPKeepAlive feature doesn't work well with ToxyProxy. Is it essential setting for the tests?

- also added unit tests

congoamz · 2023-05-18T22:23:09Z

wrapper/src/main/java/software/amazon/jdbc/util/CacheMap.java

  }

  public V get(final K key) {
-    final CacheItem<V> cacheItem = cache.computeIfPresent(key, (kk, vv) -> vv.isExpired() ? null : vv);
+    removeIfExpired(key);
+    final CacheItem cacheItem = cache.get(key);
    return cacheItem == null ? null : cacheItem.item;
  }

  public V get(final K key, final V defaultItemValue, final long itemExpirationNano) {


do we need both this and computeIfAbsent? They are quite similar. Differences: computeIfAbsent calls cleanup and extends the expiration

congoamz · 2023-05-18T22:23:37Z

wrapper/src/main/java/software/amazon/jdbc/util/CacheMap.java

    return cacheItem.item;
  }

+  public V getWithExtendExpiration(final K key, final long itemExpirationNano) {


only used in tests, should we remove?

congoamz · 2023-05-18T22:23:48Z

wrapper/src/main/java/software/amazon/jdbc/util/CacheMap.java

    cleanUp();
+    removeAndDispose(key);
+    cache.put(key, new CacheItem(item, System.nanoTime() + itemExpirationNano));
  }

  public void putIfAbsent(final K key, final V item, final long itemExpirationNano) {


only used in tests, should we remove?

congoamz · 2023-05-18T22:25:28Z

wrapper/src/main/java/software/amazon/jdbc/util/CacheMap.java

+      final IsItemValidFunc<V> isItemValidFunc,
+      final ItemDisposalFunc<V> itemDisposalFunc) {
+    this.isItemValidFunc = isItemValidFunc;
+    this.itemDisposalFunc = itemDisposalFunc;
  }

  public V get(final K key) {


Should we call cleanup() in the get functions or any other function? Why do we call it only in certain functions?

congoamz · 2023-05-18T22:38:31Z

wrapper/src/main/java/software/amazon/jdbc/util/CacheMap.java

-  private final Map<K, CacheItem<V>> cache = new ConcurrentHashMap<>();
-  private final long cleanupIntervalNanos = TimeUnit.MINUTES.toNanos(10);
+  private final Map<K, CacheItem> cache = new ConcurrentHashMap<>();
+  private long cleanupIntervalNanos = TimeUnit.MINUTES.toNanos(10);


Should this be configurable via the HikariPooledConnectionProvider constructor? Right now there isn't a way for the user to set this

…tionMap javadocs

sergiyvamz · 2023-05-24T15:56:20Z

wrapper/src/main/java/software/amazon/jdbc/util/SlidingExpirationCache.java

+   * @param key the key associated with the value to be removed/disposed
+   */
+  public void remove(final K key) {
+    cleanUp();


I'd change order of this two lines. In such way, cleanUp() will have less item to check.

sergiyvamz · 2023-05-24T16:07:40Z

wrapper/src/main/java/software/amazon/jdbc/util/SlidingExpirationCache.java

+   * @return the current (existing or computed) value associated with the specified key, or null if
+   *     the computed value is null
+   */
+  public V computeIfAbsent(


Should we remove similar method (and sliding expiration related functionality) from CacheMap<> ?

karenc-bq · 2023-05-24T18:33:59Z

docs/using-the-jdbc-driver/using-plugins/UsingTheReadWriteSplittingPlugin.md

@@ -39,7 +39,9 @@ The wrapper driver currently uses [Hikari](https://github.com/brettwooldridge/Hi
 - username
 - password

-You can optionally pass in a `HikariPoolMapping` function as a second parameter to the `HikariPooledConnectionProvider`. This allows you to decide when new connection pools should be created by defining what is included in the pool map key. A new pool will be created each time a new connection is requested with a unique key. By default, a new pool will be created for each unique instance-user combination. If you would like to define a different key system, you should pass in a `HikariPoolMapping` function defining this logic. Note that the user will always be automatically included in the key for security reasons. Please see [ReadWriteSplittingPostgresExample.java](../../../examples/AWSDriverExample/src/main/java/software/amazon/ReadWriteSplittingPostgresExample.java) for an example of how to configure the pool map key.
+You can optionally pass in a `HikariPoolMapping` function as a second parameter to the `HikariPooledConnectionProvider`. This allows you to decide when new connection pools should be created by defining what is included in the pool map key. A new pool will be created each time a new connection is requested with a unique key. By default, a new pool will be created for each unique instance-user combination. If you would like to define a different key system, you should pass in a `HikariPoolMapping` function defining this logic. Please see [ReadWriteSplittingPostgresExample.java](../../../examples/AWSDriverExample/src/main/java/software/amazon/ReadWriteSplittingPostgresExample.java) for an example of how to configure the pool map key.


Can we provide a simple example here for the hikaripoolmapping function, and say, for the full example please see ReadWriteSplittingPostgresExample.java

props.setProperty("somePropertyValue", "1"); // used in getPoolKey final HikariPooledConnectionProvider connProvider = new HikariPooledConnectionProvider( ReadWriteSplittingPostgresExample::getHikariConfig, ReadWriteSplittingPostgresExample::getPoolKey ); ConnectionProviderManager.setConnectionProvider(connProvider); private static String getPoolKey(HostSpec hostSpec, Properties props) { // Include the URL, user, and somePropertyValue in the connection pool key so that a new // connection pool will be opened for each different instance-user-somePropertyValue // combination. final String user = props.getProperty(PropertyDefinition.USER.name); final String somePropertyValue = props.getProperty("somePropertyValue"); return hostSpec.getUrl() + user + somePropertyValue; }

karenc-bq · 2023-05-24T19:56:38Z

wrapper/src/main/resources/messages.properties

 # Read Write Splitting Plugin
 ReadWriteSplittingPlugin.setReadOnlyOnClosedConnection=setReadOnly cannot be called on a closed connection.
+ReadWriteSplittingPlugin.errorSwitchingToCachedReader=An error occurred while trying to switch to a cached reader connection to ''{0}''. The driver will attempt to establish a new reader connection.
+ReadWriteSplittingPlugin.errorSwitchingToCachedReaderWithCause=An error occurred while trying to switch to a cached reader connection to ''{0}''. Error message: ''{1}''. The driver will attempt to establish a new reader connection.


Suggested change

ReadWriteSplittingPlugin.errorSwitchingToCachedReaderWithCause=An error occurred while trying to switch to a cached reader connection to ''{0}''. Error message: ''{1}''. The driver will attempt to establish a new reader connection.

ReadWriteSplittingPlugin.errorSwitchingToCachedReaderWithCause=An error occurred while trying to switch a cached reader connection to ''{0}''. Error message: ''{1}''. The driver will attempt to establish a new reader connection.

I think the error message is misleading if we remove 'to'. Removing it makes it sound like the error occurred when we tried to change a cached reader connection to another reader. The actual scenario is that we tried to switch from a writer to a cached reader. I'll rephrase the error message a bit to hopefully be clearer

karenc-bq · 2023-05-24T19:56:53Z

wrapper/src/main/resources/messages.properties

 # Read Write Splitting Plugin
 ReadWriteSplittingPlugin.setReadOnlyOnClosedConnection=setReadOnly cannot be called on a closed connection.
+ReadWriteSplittingPlugin.errorSwitchingToCachedReader=An error occurred while trying to switch to a cached reader connection to ''{0}''. The driver will attempt to establish a new reader connection.


Suggested change

ReadWriteSplittingPlugin.errorSwitchingToCachedReader=An error occurred while trying to switch to a cached reader connection to ''{0}''. The driver will attempt to establish a new reader connection.

ReadWriteSplittingPlugin.errorSwitchingToCachedReader=An error occurred while trying to switch a cached reader connection to ''{0}''. The driver will attempt to establish a new reader connection.

wrapper/src/test/java/integration/refactored/container/ConnectionStringHelper.java

wrapper/src/test/java/integration/refactored/container/tests/AutoscalingTests.java

karenc-bq · 2023-05-24T20:23:13Z

wrapper/src/test/java/integration/refactored/container/tests/AutoscalingTests.java

+        final Connection conn1 = DriverManager.getConnection(connString, props);
+        connections.add(conn1);
+        final Connection conn2 = DriverManager.getConnection(connString, props);
+        connections.add(conn2);


Suggested change

final Connection conn1 = DriverManager.getConnection(connString, props);

connections.add(conn1);

final Connection conn2 = DriverManager.getConnection(connString, props);

connections.add(conn2);

// Create 2 connections per instance.

connections.add(DriverManager.getConnection(connString, props));

connections.add(DriverManager.getConnection(connString, props));

aaron-congo and others added 8 commits May 10, 2023 17:36

Add leastConnections strategy and autoscaling functionality

77d167d

Add MakeSureFirstInstanceWriter

e7370bb

use CacheMap to store connection pools;

ee0d392

implement computeIfAbsent() for CacheMap; provide dispose callback function to properly close DataSource;

wip - resolve CacheMap enhancements

64c1f8c

Autoscaling tests passing

49c084b

Remove notifyNodeListChanged from rw plugin

6f25a83

Cleanup

b9f3151

More cleanup

9c900a7

hsuamz changed the title ~~[WIP] Implement autoscaling and the leastConnections strategy~~ [WIP] feat: Implement autoscaling and the leastConnections strategy May 17, 2023

aaron-congo added 2 commits May 17, 2023 14:14

Add HikariConnectionProvider unit tests

414a55a

More tests

d73bddd

sergiyvamz reviewed May 18, 2023

View reviewed changes

aaron-congo added 3 commits May 18, 2023 14:18

Dispose of items when they are removed from the CacheMap

85d299b

- also added unit tests

Fix unit tests for HikariPooledConnectionProvider

55c4c33

Refactor duplicate code in CacheMap

1fefe13

congoamz commented May 18, 2023

View reviewed changes

aaron-congo added 2 commits May 19, 2023 11:29

SlidingExpirationMap wip

a0940ce

Merge remote-tracking branch 'upstream/main' into leastConnections

a5e08f3

congoamz changed the title ~~[WIP] feat: Implement autoscaling and the leastConnections strategy~~ feat: Implement autoscaling and the leastConnections strategy May 19, 2023

aaron-congo added 2 commits May 19, 2023 15:22

Change HikariPooledConnectionProvider constructors, add SlidingExpira…

e569c8e

…tionMap javadocs

Fix checkstyle

1fe5669

congoamz changed the title ~~feat: Implement autoscaling and the leastConnections strategy~~ feat: implement autoscaling and the leastConnections strategy May 23, 2023

aaron-congo added 2 commits May 23, 2023 10:17

cleanup

8d92175

Remove keepalive from ReadWriteSplittingTests

802e5e0

Increase create/delete instance timeout for autoscaling tests

290c3be

sergiyvamz reviewed May 24, 2023

View reviewed changes

sergiyvamz approved these changes May 24, 2023

View reviewed changes

PR Suggestions

d55a1ff

karenc-bq reviewed May 24, 2023

View reviewed changes

wrapper/src/test/java/integration/refactored/container/ConnectionStringHelper.java Show resolved Hide resolved

karenc-bq reviewed May 24, 2023

View reviewed changes

wrapper/src/test/java/integration/refactored/container/tests/AutoscalingTests.java Show resolved Hide resolved

karenc-bq reviewed May 24, 2023

View reviewed changes

PR suggestions

ebe85e1

karenc-bq approved these changes May 24, 2023

View reviewed changes

congoamz merged commit 4885da5 into aws:main May 24, 2023

aaron-congo deleted the leastConnections branch May 24, 2023 22:42

hsuamz mentioned this pull request Jun 19, 2023

Auto-scaling support for Aurora Postgres #273

Closed

2 tasks

	ReadWriteSplittingPlugin.errorSwitchingToCachedReaderWithCause=An error occurred while trying to switch to a cached reader connection to ''{0}''. Error message: ''{1}''. The driver will attempt to establish a new reader connection.
	ReadWriteSplittingPlugin.errorSwitchingToCachedReaderWithCause=An error occurred while trying to switch a cached reader connection to ''{0}''. Error message: ''{1}''. The driver will attempt to establish a new reader connection.

feat: implement autoscaling and the leastConnections strategy #451

feat: implement autoscaling and the leastConnections strategy #451

Uh oh!

Conversation

congoamz commented May 17, 2023

Summary

Description

Additional Reviewers

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergiyvamz May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sergiyvamz May 18, 2023 •

edited

Loading