feat: introduce target driver dialects #452

sergiyvamz · 2023-05-17T18:56:21Z

Summary

Introduce target driver dialects

Additional Reviewers

@karenc-bq
@aaron-congo
@davecramer

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/targetdriverdialect/PgDataSourceHelper.java

karenc-bq · 2023-05-24T22:20:21Z

wrapper/src/main/java/software/amazon/jdbc/targetdriverdialect/GenericTargetDriverDialect.java

+      customProps.setProperty(urlPropertyName, finalUrl);
+    }
+
+    LOGGER.finest(() -> PropertyUtils.logProperties(customProps, "Connecting with properties: \n"));


Can we log the properties and the url in one message? It helps the debugging process in multithreaded applications

Hmm they are different properties. Url may be missed. Let's find a separate solution for multithreaded logs.

wrapper/src/main/java/software/amazon/jdbc/targetdriverdialect/MariadbDataSourceHelper.java

wrapper/src/main/java/software/amazon/jdbc/targetdriverdialect/PgTargetDriverDialect.java

aaronchung-bitquill · 2023-05-24T22:29:04Z

wrapper/src/main/java/software/amazon/jdbc/targetdriverdialect/PgTargetDriverDialect.java

+      final @Nullable String databasePropertyName) throws SQLException {
+
+    // The logic is isolated to a separated class since it uses
+    // direct reference to org.postgresql.ds.common.BaseDataSource


Referring to the code comments above, is the idea to decouple the Dialect class from org.postgresql.ds.common.BaseDataSource?

That's correct. The idea is to decouple different dialect classes. Since PgTargetDriverDialect is instantiated in TargetDriverDialectManager, it's impossible to use any classes from org.postgresql package in PgTargetDriverDialect. In this case AWS JDBC Driver would have a dependency on org.postgresql package. That's why all logic is isolated in PgDataSourceHelper class. This helper class has direct references to org.postgresql package but it's instantiated when it's needed only, i.e. when the target driver is actually pgjdbc driver.

Thank you for the explanation!

documentation jacoco min coverage code style fix issue with MariaDb driver for Mysql protocol add checkstyle import exceptions introduce target driver dialects

aaronchung-bitquill · 2023-05-25T00:09:49Z

wrapper/src/main/java/software/amazon/jdbc/targetdriverdialect/TargetDriverDialectManager.java

+   * Dialect objects are shared between different connections.
+   * The order of entries in this map is the order in which dialects are called/verified.
+   */
+  protected static final Map<String, TargetDriverDialect> knownDialectsByCode =


Nit: would it be better to have this mapping located in the TargetDriverDialectCodes class? That way if a new dialect is introduced, it is one less class that needs to be updated, and it is harder for the developer to forget updating this mapping.

Also sorry about the lateness of the comment.

congoamz · 2023-05-24T23:52:44Z

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

   */
+  @Deprecated
  Connection connect(@NonNull String url, @NonNull Properties props)
      throws SQLException; // TODO: this method is only called by tests/benchmarks and can likely be deprecated


Should we remove the comment now that this is deprecated or no?

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

congoamz · 2023-05-25T00:23:06Z

wrapper/src/main/java/software/amazon/jdbc/exceptions/GenericExceptionHandler.java

    Throwable exception = throwable;

    while (exception != null) {
+      if (exception instanceof SQLLoginException) {


nit: should we move this check to isLoginException?

congoamz · 2023-05-25T00:27:39Z

wrapper/src/main/java/software/amazon/jdbc/plugin/AwsSecretsManagerConnectionPlugin.java

  protected PluginService pluginService;

+  static {
+    PropertyDefinition.registerPluginProperties(AwsSecretsManagerConnectionPlugin.class);


Would it work to register the properties for all available plugins automatically in the ConnectionPluginManager so that we don't need to add this block to every plugin? Might not be necessary but it would be nice to avoid the need to add this to every plugin if possible

Registering properties is direct responsibility of a particular plugin. Register properties for all know plugins in ConnectionPluginManager doesn't look like direct responsibilities of the this class. Also, such approach doesn't cover user custom plugins. I'd prefer to let plugins be responsible for such registration.

congoamz · 2023-05-25T00:35:48Z

wrapper/src/main/java/software/amazon/jdbc/targetdriverdialect/GenericTargetDriverDialect.java

+    }
+
+    if (hostSpec.isPortSpecified() && !isNullOrEmpty(portPropertyName)) {
+      customProps.put(portPropertyName, hostSpec.getPort());


should this be setProperty instead of put?

Technically it's the same call to put(). I'll fix it for consistency.

wrapper/src/main/java/software/amazon/jdbc/targetdriverdialect/MariadbTargetDriverDialect.java

congoamz · 2023-05-25T00:49:59Z

wrapper/src/main/java/software/amazon/jdbc/targetdriverdialect/TargetDriverDialectCodes.java

+public class TargetDriverDialectCodes {
+  public static final String PG_JDBC = "pgjdbc";
+  public static final String MYSQL_CONNECTOR_J = "mysql-connector-j";
+  public static final String MARIADB_CONNECTOR_J_VER_3 = "mariadb-connector-j-3"; // ver 3.0.3+


just curious, why do we need the version in this string?

There's a chance that different versions of the same driver may require different dialects. MariaDb driver changed behaviour in version 3+. So I think it makes sense to be explicit in defining dialect codes.

wrapper/src/main/resources/messages.properties

sergiyvamz force-pushed the target-driver-dialects branch from de7b877 to bb64f60 Compare May 18, 2023 22:21

sergiyvamz changed the title ~~[WIP] feat: introduce target driver dialects~~ feat: introduce target driver dialects May 23, 2023

aaronchung-bitquill reviewed May 24, 2023

View reviewed changes

wrapper/src/main/java/software/amazon/jdbc/targetdriverdialect/PgDataSourceHelper.java Outdated Show resolved Hide resolved

karenc-bq reviewed May 24, 2023

View reviewed changes

wrapper/src/main/java/software/amazon/jdbc/targetdriverdialect/MariadbDataSourceHelper.java Outdated Show resolved Hide resolved

aaronchung-bitquill reviewed May 24, 2023

View reviewed changes

wrapper/src/main/java/software/amazon/jdbc/targetdriverdialect/PgTargetDriverDialect.java Outdated Show resolved Hide resolved

aaronchung-bitquill reviewed May 24, 2023

View reviewed changes

karenc-bq approved these changes May 24, 2023

View reviewed changes

sergiyvamz force-pushed the target-driver-dialects branch from 7f06bff to 8039457 Compare May 24, 2023 23:26

code review

38abe6f

documentation jacoco min coverage code style fix issue with MariaDb driver for Mysql protocol add checkstyle import exceptions introduce target driver dialects

sergiyvamz force-pushed the target-driver-dialects branch from 8039457 to 38abe6f Compare May 24, 2023 23:29

code review

b1dbe8e

aaronchung-bitquill reviewed May 25, 2023

View reviewed changes

congoamz reviewed May 25, 2023

View reviewed changes

sergiyv-improving added 2 commits May 25, 2023 10:13

code review

76508af

code review

a49bf59

sergiyvamz merged commit 811a454 into main May 26, 2023

sergiyvamz deleted the target-driver-dialects branch May 26, 2023 00:03

karenc-bq mentioned this pull request Jun 14, 2023

remove compileOnly dependencies for postgres and mariadb #464

Open

feat: introduce target driver dialects #452

feat: introduce target driver dialects #452

Uh oh!

Conversation

sergiyvamz commented May 17, 2023

Summary

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronchung-bitquill May 25, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aaronchung-bitquill May 25, 2023 •

edited

Loading