HIVE-25667: Unify code managing JDBC databases in tests #2919

mark-bathori · 2022-01-04T17:54:05Z

What changes were proposed in this pull request?

Currently there are two class hierarchies managing JDBC databases in tests, DatabaseRule and AbstractExternalDB. These changes meant to unify these database related functionalities and get rid of the duplicated codes.

Why are the changes needed?

The current solution is redundant, contains a lot of code duplications. The unified database handling gives easier extensibility and modification.

Does this PR introduce any user-facing change?

No

How was this patch tested?

All the previous tests that were covering the affected database functionality are passing with the same expected results.

zabetak

Many thanks for picking this up @mbathori-cloudera ! I went quickly over the PR and left a few comments.

zabetak · 2022-01-10T11:26:56Z

...astore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/AbstractDatabase.java

+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+public abstract class AbstractDatabase extends ExternalResource {


Do we want to keep this dependent to ExternalResource and junit APis in general?

zabetak · 2022-01-10T11:28:48Z

...astore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/AbstractDatabase.java

@@ -0,0 +1,302 @@
+package org.apache.hadoop.hive.metastore.dbinstall;


Isn't hive-testutils module a better place for keeping these classes since they are not only used in the metastore-server?

Yes I felt that the metastore isn’t the right place for this class but didn't know where should it be, I’ll move it into the hive-testutils module.

The hive-testutils module seems to be the right place for this classes we should be careful though not to create circular dependencies between hive-testutils and metastore modules.

zabetak · 2022-01-10T11:31:22Z

...astore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/AbstractDatabase.java

+  public void install() {
+    createUser();
+    installLatest();
+  }
+
+  public int createUser() {
+    return new MetastoreSchemaTool().setVerbose(verbose).run(new String[] {
+        "-createUser",
+        "-dbType", getDbType(),
+        "-userName", getDbRootUser(),
+        "-passWord", getDbRootPassword(),
+        "-hiveUser", getHiveUser(),
+        "-hivePassword", getHivePassword(),
+        "-hiveDb", getDb(),
+        "-url", getInitialJdbcUrl(),
+        "-driver", getJdbcDriver()
+    });
+  }
+
+  public int installLatest() {
+    return new MetastoreSchemaTool().setVerbose(verbose).run(new String[] {
+        "-initSchema",
+        "-dbType", getDbType(),
+        "-userName", getHiveUser(),
+        "-passWord", getHivePassword(),
+        "-url", getJdbcUrl(),
+        "-driver", getJdbcDriver(),
+        "-verbose"
+    });
+  }
+
+  public int installAVersion(String version) {
+    return new MetastoreSchemaTool().setVerbose(verbose).run(new String[] {
+        "-initSchemaTo", version,
+        "-dbType", getDbType(),
+        "-userName", getHiveUser(),
+        "-passWord", getHivePassword(),
+        "-url", getJdbcUrl(),
+        "-driver", getJdbcDriver()
+    });
+  }
+
+  public int upgradeToLatest() {
+    return new MetastoreSchemaTool().setVerbose(verbose).run(new String[] {
+        "-upgradeSchema",
+        "-dbType", getDbType(),
+        "-userName", getHiveUser(),
+        "-passWord", getHivePassword(),
+        "-url", getJdbcUrl(),
+        "-driver", getJdbcDriver()
+    });
+  }
+
+  public int validateSchema() {
+    return new MetastoreSchemaTool().setVerbose(verbose).run(new String[] {
+        "-validate",
+        "-dbType", getDbType(),
+        "-userName", getHiveUser(),
+        "-passWord", getHivePassword(),
+        "-url", getJdbcUrl(),
+        "-driver", getJdbcDriver()
+    });
+  }


Possibly we could do this in separate refactoring/jira but these methods are not necessary for all consumers of this API so we could put them in a separate class/interface.

zabetak · 2022-01-10T11:33:55Z

...e/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Mssql.java

@@ -63,7 +64,8 @@ public String getJdbcDriver() {

  @Override
  public String getJdbcUrl(String hostAddress) {
-    return "jdbc:sqlserver://" + hostAddress + ":1433;DatabaseName=" + HIVE_DB + ";";
+    //Mssql doesn't support database parameter in docker


I don't understand what the comment means. Can you elaborate more?

The Mssql docker init doesn’t have database parameter for initialisation like postgres/mysql etc. so the initial jdbc connection should go to default url if the database was created with docker database argument.(microsoft/mssql-docker#2)

zabetak · 2022-01-10T11:41:27Z

...astore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/AbstractDatabase.java

+  @Override
+  public void before() throws Exception {
+    launchDockerContainer();
+    MetastoreSchemaTool.setHomeDirForTesting();
+  }
+
+  @Override
+  public void after() {
+    if ("true".equalsIgnoreCase(System.getProperty("metastore.itest.no.stop.container"))) {
+      LOG.warn("Not stopping container " + getDockerContainerName() + " at user request, please "
+          + "be sure to shut it down before rerunning the test.");
+      return;
+    }
+    try {
+      cleanupDockerContainer();
+    } catch (InterruptedException | IOException e) {
+      e.printStackTrace();
+    }
+  }


Possibly these methods can go to those classes using this class as a rule. Another alternative would be to create a rule class which wraps an instance of AbstractDatabase and provides these methods.

zabetak · 2022-01-10T11:49:05Z

...metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Mariadb.java

+
+  @Override
+  public List<String> getDockerBaseArgs() {
+    return new ArrayList(Arrays.asList("-p", "3309:3306",


Why are we wrapping into ArrayList? Are we going to modify the list? Keeping things immutable makes the code easier to understand and use.

I was wrapping this because further database argumentum can be added in the AbstractDatabase class, so the list can’t be immutable. The AbstractDatabse.getDockerAdditionalArgs() method can modify the list.

I think that we could create a new list inside AbstractDatabse.getDockerAdditionalArgs() if necessary instead of modifying those that we obtain from the getters.

zabetak · 2022-01-10T11:51:47Z

...e/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/rules/Derby.java

@@ -42,7 +46,7 @@ public String getDockerImageName() {
  }

  @Override
-  public String[] getDockerAdditionalArgs() {
+  public List<String> getDockerBaseArgs() {
    return null;


Effective Java Item 43: Return empty arrays or collections, not nulls

...astore-server/src/test/java/org/apache/hadoop/hive/metastore/dbinstall/AbstractDatabase.java

-Moved common database classes to hive-testutils -Moved metastore related parts from common database classes to wrapper class in metastore -Removed ExternalResource and refactored depending functions

mark-bathori · 2022-01-16T20:14:06Z

@zabetak Thanks for reviewing my changes. I made a new commit where I tried to address all of your comments.

I moved the common database classes to the hive-testutils module.
Moved metastore related parts from common database classes to a wrapper class in the metastore.
Removed the ExternalResource and refactored depending functions.

Some note for the hive-testutil pom changes:
I had a strange issue while adding hive-testutils to the itests/util/pom.xml. The qtests were not executed and the build automatically skipped all of them without any error message. It turned out that a dependency called junit-jupiter-api caused this issue. If I set this dependency to test scope the problem would be solved but I can’t do it because classes that are using it are placed in the src package. The easiest solution was to set the dependency to optional so it will be excluded.

mark-bathori · 2022-01-16T21:43:07Z

It seems like the metastore verify stage is failing in the pipeline because I refactored the DbInstall tests and it still tries to run the ITestDerby,ITestMysql and ITestPostgres but they don’t exist anymore. All of these tests are now running in ITestDbInstall. I’m not sure if the pipeline needs to be adjusted to the changes or should I restore the original form of these tests.

github-actions · 2022-03-18T00:16:23Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.

zabetak · 2022-03-20T09:34:16Z

Hey bot please keep this PR open!

github-actions · 2022-05-20T00:18:53Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.

HIVE-25667: Unify code managing JDBC databases in tests

ede0218

kgyrtkirk added tests pending tests unstable and removed tests pending labels Jan 4, 2022

Fix test failures

634c57a

kgyrtkirk added tests pending tests passed and removed tests unstable tests pending labels Jan 5, 2022

zabetak reviewed Jan 10, 2022

View reviewed changes

Addressing review comments

513365d

-Moved common database classes to hive-testutils -Moved metastore related parts from common database classes to wrapper class in metastore -Removed ExternalResource and refactored depending functions

kgyrtkirk added tests pending tests failed and removed tests passed tests pending labels Jan 16, 2022

Merge branch 'master' into HIVE-25667

20aa85d

kgyrtkirk added tests pending and removed tests failed labels Jan 16, 2022

kgyrtkirk added tests failed and removed tests pending labels Jan 16, 2022

github-actions bot added the stale label Mar 18, 2022

github-actions bot removed the stale label Mar 21, 2022

github-actions bot added the stale label May 20, 2022

github-actions bot closed this May 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIVE-25667: Unify code managing JDBC databases in tests #2919

HIVE-25667: Unify code managing JDBC databases in tests #2919

mark-bathori commented Jan 4, 2022

zabetak left a comment

zabetak Jan 10, 2022

zabetak Jan 10, 2022

mark-bathori Jan 10, 2022

zabetak Jan 13, 2022

zabetak Jan 10, 2022

zabetak Jan 10, 2022

mark-bathori Jan 10, 2022

zabetak Jan 10, 2022

zabetak Jan 10, 2022

mark-bathori Jan 10, 2022 •

edited

Loading

zabetak Jan 13, 2022

zabetak Jan 10, 2022

mark-bathori commented Jan 16, 2022

mark-bathori commented Jan 16, 2022 •

edited

Loading

github-actions bot commented Mar 18, 2022

zabetak commented Mar 20, 2022

github-actions bot commented May 20, 2022

		@@ -0,0 +1,302 @@
		package org.apache.hadoop.hive.metastore.dbinstall;

HIVE-25667: Unify code managing JDBC databases in tests #2919

HIVE-25667: Unify code managing JDBC databases in tests #2919

Conversation

mark-bathori commented Jan 4, 2022

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

zabetak left a 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

mark-bathori Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-bathori commented Jan 16, 2022

mark-bathori commented Jan 16, 2022 • edited Loading

github-actions bot commented Mar 18, 2022

zabetak commented Mar 20, 2022

github-actions bot commented May 20, 2022

mark-bathori Jan 10, 2022 •

edited

Loading

mark-bathori commented Jan 16, 2022 •

edited

Loading