sort plugins by default #542

sergiyvamz · 2023-07-12T19:51:21Z

Summary

Sort plugins by default to prevent plugin misconfiguration. Allows a user to provide a custom plugin order if needed.

Description

Plugin chain initialization has been separated to ConnectionPluginChainBuilder class. Depending on user settings, a plugin factory list is sorted based on plugin assigned weights.

Additional Reviewers

@karenc-bq @aaron-congo @aaronchung-bitquill @crystall-bitquill

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

brunos-bq · 2023-07-12T22:54:25Z

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

+
+  /**
+   * Plugins are sorted by assigned weight ascending.
+   * The first plugin in the final sorted list has a minimal weight.


I would suggest to reword this comment in a clearer and simpler way.

Ex: "The final list of plugins will be sorted by weight, starting from the lowest values up to the highest values.
The first plugin of the list will have the lowest weight, and the last one will have the highest weight."

brunos-bq · 2023-07-12T22:54:27Z

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

+          put(IamAuthConnectionPluginFactory.class, 900);
+          put(AwsSecretsManagerConnectionPluginFactory.class, 1000);
+          put(LogQueryConnectionPluginFactory.class, 1100);
+          put(ConnectTimeConnectionPluginFactory.class, STICK_TO_PRIOR_PLUGIN);


This variable has control value in the code below, but also works as a value? This can be confusing.
Also, since the value is at -1 for now, does that mean that those 3 plugins should always go first in the list?
Why not using 000 that "kinda" follows the notation used for the other plugins?

I need a weight as a number. In this case it's easier to sort elements in the list. 000 is not good for numbers. I use a constant value -1 to distinguish elements with absolute weights and elements with relative weight. The code below support such notation.

brunos-bq · 2023-07-12T22:55:44Z

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

+
+  protected static final String DEFAULT_PLUGINS = "auroraConnectionTracker,failover,efm";
+
+  private static class PluginFactoryInfo {


NIT: Let's add a comment explaining what this internal class does/means (even though it is very straight-forward)

brunos-bq · 2023-07-12T23:01:15Z

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

+                  .map(Class::getSimpleName)
+                  .collect(Collectors.joining(", ")));
+      }
+


is it worth to separate the code below into a function loadFactories() that returns the factories list?

brunos-bq · 2023-07-12T23:04:42Z

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

+        lastWeight = info.weight;
+        continue;
+      }
+      lastWeight++;


this bit is a bit confusing - why do we need to increment and reassign weights?

Reworked this piece. Replaced two loops with a single one. Added more comments to explain the algorithm.

brunos-bq · 2023-07-12T23:11:51Z

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

@@ -48,6 +48,12 @@ public class PropertyDefinition {
      new AwsWrapperProperty(
          "wrapperPlugins", null, "Comma separated list of connection plugin codes");

+  public static final AwsWrapperProperty PRESERVE_PLUGINS_ORDER =


This is an interesting one.
How do we put this plugin order thing in the documentation?
If we want the sorting to be on by default, why not revert the wording from PRESERVE_PLUGINS_ORDER to AUTOMATICALLY_SORT_PLUGINS/AUTO_SORT_PLUGIN_ORDER ?

In this way, we could add something in the docs like this:
"This flag is enabled by default, meaning that the plugins order will be auto generated according to the order that we expect the plugins to be ordered. Disable it at your own risk or if you really need some plugins to be executed in a particular order"

karenc-bq · 2023-07-13T16:47:18Z

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

+
+        // make a chain of connection plugins
+
+        plugins = new ArrayList<>(factories.length + 1);


What is the value of initialize the plugins arraylist here with a fixed size? Can we just initialize it on line 118
final List<ConnectionPlugin> plugins = new ArrayList<>(); and remove the if-else clause.

Using new ArrayList() will initialize the list with 0 capacity. Every time we need to add a new element to the list will require the list to expand. Since ArrayList is based on internal array it means that an internal copy from one array to a new array will occur. By specifying an initial size we can avoid unnecessary overhead with copying.

karenc-bq · 2023-07-13T16:47:31Z

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

+  protected List<Class<? extends ConnectionPluginFactory>> sortPluginFactories(
+      final List<Class<? extends ConnectionPluginFactory>> unsortedPluginFactories) {
+
+    ArrayList<PluginFactoryInfo> weights = new ArrayList<>();


Suggested change

ArrayList<PluginFactoryInfo> weights = new ArrayList<>();

final List<PluginFactoryInfo> weights = new ArrayList<>();

brunos-bq · 2023-07-13T20:41:10Z

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

+
+  private static final Logger LOGGER = Logger.getLogger(ConnectionPluginChainBuilder.class.getName());
+
+  private static final int WEIGHT_RELATIVE_TO_PRIOR_PLUGIN = -1;


NIT: i would just add a comment that plugins that have this value in the map will not be affected by any reordering

sergiyvamz changed the title ~~add plugin order sort by default~~ sort plugins by default Jul 12, 2023

brunos-bq reviewed Jul 12, 2023

View reviewed changes

karenc-bq reviewed Jul 13, 2023

View reviewed changes

brunos-bq approved these changes Jul 13, 2023

View reviewed changes

karenc-bq approved these changes Jul 13, 2023

View reviewed changes

sergiyvamz force-pushed the plugins-order branch from 1657792 to 13eb278 Compare July 31, 2023 16:24

sergiyvamz enabled auto-merge (squash) July 31, 2023 17:35

sergiyvamz disabled auto-merge July 31, 2023 17:57

sergiyvamz added 2 commits July 31, 2023 13:40

add plugin order sort by default

51af028

code review

1eb5802

sergiyvamz force-pushed the plugins-order branch from 13eb278 to 1eb5802 Compare July 31, 2023 20:40

sergiyvamz enabled auto-merge (squash) July 31, 2023 21:46

sergiyvamz merged commit 73a5abc into main Jul 31, 2023

sergiyvamz deleted the plugins-order branch July 31, 2023 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sort plugins by default #542

sort plugins by default #542

sergiyvamz commented Jul 12, 2023 •

edited

Loading

brunos-bq Jul 12, 2023

brunos-bq Jul 12, 2023

sergiyvamz Jul 13, 2023

brunos-bq Jul 12, 2023

brunos-bq Jul 12, 2023

brunos-bq Jul 12, 2023

sergiyvamz Jul 13, 2023

brunos-bq Jul 12, 2023

karenc-bq Jul 13, 2023

sergiyvamz Jul 13, 2023

karenc-bq Jul 13, 2023

brunos-bq Jul 13, 2023


		protected static final String DEFAULT_PLUGINS = "auroraConnectionTracker,failover,efm";

		private static class PluginFactoryInfo {


		// make a chain of connection plugins

		plugins = new ArrayList<>(factories.length + 1);

	ArrayList<PluginFactoryInfo> weights = new ArrayList<>();
	final List<PluginFactoryInfo> weights = new ArrayList<>();


		private static final Logger LOGGER = Logger.getLogger(ConnectionPluginChainBuilder.class.getName());

		private static final int WEIGHT_RELATIVE_TO_PRIOR_PLUGIN = -1;

sort plugins by default #542

sort plugins by default #542

Conversation

sergiyvamz commented Jul 12, 2023 • edited Loading

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.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiyvamz commented Jul 12, 2023 •

edited

Loading