refactor: host alias logic in wrapper #431

karenc-bq · 2023-05-05T18:49:58Z

Summary

Integration Test Run

https://github.com/Bit-Quill/aws-advanced-jdbc-wrapper/actions/runs/4920404497

Description

Issues fixed

After failover Aurora Connection Tracker incorrectly invalidates the newly promoted writer
After failover Aurora Connection Tracker incorrectly invalidates a random reader that is neither the old or new writer.
If the initial connection is established using an cluster endpoint, EFM plugin uses the cluster endpoint as the monitoring endpoint

Issue 1 Solution

Issue one is caused by the following check.

  private boolean isRoleChanged(final EnumSet<NodeChangeOptions> changes) {
    return changes.contains(NodeChangeOptions.PROMOTED_TO_WRITER)
        || changes.contains(NodeChangeOptions.PROMOTED_TO_READER);
  }

We need to remove the check for PROMOTED_TO_WRITER

Issue 2 & 3

Issues 2 and 3 are both caused by missing or incorrect instance endpoint in the hostspec's aliases. Issues 2 and 3 were originally fixed in #428 and #428, and the fixes contain a lot of similarities. This refactoring centralizes the logic populating the aliases and make it less error prone.

More information in respective tickets:
https://github.com/orgs/awslabs/projects/91/views/13?pane=issue&itemId=27190320
https://github.com/orgs/awslabs/projects/91/views/13?pane=issue&itemId=27190427

Additional Reviewers

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

wrapper/src/main/java/software/amazon/jdbc/PluginServiceImpl.java

wrapper/src/main/java/software/amazon/jdbc/hostlistprovider/AuroraHostListProvider.java

sergiyvamz · 2023-05-05T19:13:58Z

wrapper/src/main/java/software/amazon/jdbc/hostlistprovider/AuroraHostListProvider.java

+      if (resultSet.next()) {
+        final String instanceName = resultSet.getString(1);
+        if (this.getCachedTopology() == null) {
+          final HostSpec host = new HostSpec(getHostEndpoint(instanceName));


Can we refresh topology with provided connection?

return null;

wrapper/src/main/java/software/amazon/jdbc/hostlistprovider/AuroraHostListProvider.java

sergiyvamz · 2023-05-05T19:15:53Z

...er/src/main/java/software/amazon/jdbc/hostlistprovider/ConnectionStringHostListProvider.java

+
+  @Override
+  public HostSpec identifyConnection(Connection connection) throws SQLException {
+    return this.hostList.get(0);


return null?

As discussed, we need to get sql from dialect, fetch node id, try to find it in topology.

sergiyvamz · 2023-05-05T19:17:31Z

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/HostMonitoringConnectionPlugin.java


    if (conn != null) {
-      generateHostAliases(driverProtocol, conn, hostSpec);
+      hostSpec.resetAliases();


Why to reset? Some other plugin might already filled aliases for this host spec.

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/MonitorServiceImpl.java

… instead of instance endpoints during status checks.

wrapper/src/main/java/software/amazon/jdbc/HostSpec.java

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/HostMonitoringConnectionPlugin.java

sergiyvamz · 2023-05-05T23:27:27Z

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/MonitorServiceImpl.java

              "MonitorServiceImpl.emptyAliasSet",
              new Object[] {hostSpec}));
-      hostSpec.addAlias(hostSpec.asAlias());
+      try {


I think we can remove this entire try-catch block. We just rely on provided nodeKeys, and if they empty then we raise an exception.

sergiyvamz · 2023-05-06T19:03:22Z

wrapper/src/main/resources/messages.properties

 PluginServiceImpl.hostAliasNotFound=Can''t find any host by the following aliases: ''{0}''.
 PluginServiceImpl.hostsChangelistEmpty=There are no changes in the hosts' availability.
+PluginServiceImpl.failedToRetrieveHostPort=Could not retrieve Host:Port for connection.
+PluginServiceImpl.nonEmptyAliases=fillAliases called when HostSpec already contains the following aliases: ''{0}''. Please reset HostSpec aliases before calling this method.


"Please reset HostSpec aliases before calling this method."

I think it may mislead developers.
"Please reset HostSpec aliases if you need to re-fill them again." ?

…ion after failover

…ion map

refactor: host alias logic in wrapper

f9d4cc0