[#2167] refactor: refactor TestGravitinoConnector to enable StaticGua…

…rdedByInstance error-prone (#2514)

<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
   Examples:
     - "[#123] feat(operator): support xxx"
     - "[#233] fix: check null before access result in xxx"
     - "[MINOR] refactor: fix typo in variable name"
     - "[MINOR] docs: fix typo in README"
     - "[#255] test: fix flaky test NameOfTheTest"
   Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. If the PR is unfinished, please mark this PR as draft.
-->

### What changes were proposed in this pull request?

`StaticGuardedByInstance` amis to avoid `public static` fields. Because
this field is not thread-safe. See more details at
https://errorprone.info/bugpattern/StaticGuardedByInstance.

In this MR, I remove some test code in
`com.datastrato.gravitino.trino.connector.GravitinoPlugin` and
`com.datastrato.gravitino.trino.connector.GravitinoConnectorFactory`. At
the same time, I enable `StaticGuardedByInstance` error-prone.

### Why are the changes needed?

- Fix: #2167 

### Does this PR introduce _any_ user-facing change?

- no

### How was this patch tested?

- original unit tests.

build.gradle.kts

@@ -338,6 +338,7 @@ subprojects {
         "SelfComparison",
         "SelfEquals",
         "SizeGreaterThanOrEqualsZero",
+        "StaticGuardedByInstance",
         "StreamToString",
         "StringBuilderInitWithChar",
         "SubstringOfZero",

trino-connector/src/main/java/com/datastrato/gravitino/trino/connector/GravitinoConnectorFactory.java

@@ -6,19 +6,22 @@
 
 import static com.datastrato.gravitino.trino.connector.GravitinoErrorCode.GRAVITINO_METALAKE_NOT_EXISTS;
 
+import com.datastrato.gravitino.client.GravitinoAdminClient;
 import com.datastrato.gravitino.trino.connector.catalog.CatalogConnectorContext;
 import com.datastrato.gravitino.trino.connector.catalog.CatalogConnectorFactory;
 import com.datastrato.gravitino.trino.connector.catalog.CatalogConnectorManager;
 import com.datastrato.gravitino.trino.connector.catalog.CatalogInjector;
 import com.datastrato.gravitino.trino.connector.system.GravitinoSystemConnector;
 import com.datastrato.gravitino.trino.connector.system.table.GravitinoSystemTableFactory;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 import io.trino.spi.TrinoException;
 import io.trino.spi.connector.Connector;
 import io.trino.spi.connector.ConnectorContext;
 import io.trino.spi.connector.ConnectorFactory;
 import java.util.Map;
+import java.util.function.Supplier;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -35,6 +38,11 @@ public String getName() {
     return DEFAULT_CONNECTOR_NAME;
   }
 
+  @VisibleForTesting
+  public CatalogConnectorManager getCatalogConnectorManager() {
+    return catalogConnectorManager;
+  }
+
   /**
    * This function call by trino creates a connector. It creates GravitinoSystemConnector at first.
    * Another time's it get GravitinoConnector by CatalogConnectorManager
@@ -61,12 +69,7 @@ public Connector create(
           catalogConnectorManager =
               new CatalogConnectorManager(catalogInjector, catalogConnectorFactory);
           catalogConnectorManager.config(config);
-
-          // For testing
-          if (GravitinoPlugin.gravitinoClient != null) {
-            catalogConnectorManager.setGravitinoClient(GravitinoPlugin.gravitinoClient);
-            GravitinoPlugin.catalogConnectorManager = catalogConnectorManager;
-          }
+          catalogConnectorManager.setGravitinoClient(clientProvider().get());
           catalogConnectorManager.start();
 
           gravitinoSystemTableFactory = new GravitinoSystemTableFactory(catalogConnectorManager);
@@ -97,4 +100,9 @@ public Connector create(
       return new GravitinoSystemConnector(metalake, catalogConnectorManager);
     }
   }
+
+  @VisibleForTesting
+  Supplier<GravitinoAdminClient> clientProvider() {
+    return () -> null;
+  }
 }

trino-connector/src/main/java/com/datastrato/gravitino/trino/connector/GravitinoPlugin.java

@@ -4,20 +4,13 @@
  */
 package com.datastrato.gravitino.trino.connector;
 
-import com.datastrato.gravitino.client.GravitinoAdminClient;
-import com.datastrato.gravitino.trino.connector.catalog.CatalogConnectorManager;
 import com.google.common.collect.ImmutableList;
 import io.trino.spi.Plugin;
 import io.trino.spi.connector.ConnectorFactory;
 
 /** Trino plugin endpoint, using java spi mechanism */
 public class GravitinoPlugin implements Plugin {
 
-  // For testing.
-  public static CatalogConnectorManager catalogConnectorManager;
-  public static GravitinoAdminClient gravitinoClient;
-
-  @Override
   public Iterable<ConnectorFactory> getConnectorFactories() {
     return ImmutableList.of(new GravitinoConnectorFactory());
   }

trino-connector/src/test/java/com/datastrato/gravitino/trino/connector/TestGravitinoConnector.java

@@ -10,6 +10,8 @@
 import static org.testng.Assert.assertEquals;
 
 import com.datastrato.gravitino.NameIdentifier;
+import com.datastrato.gravitino.client.GravitinoAdminClient;
+import com.datastrato.gravitino.trino.connector.catalog.CatalogConnectorManager;
 import io.trino.Session;
 import io.trino.plugin.memory.MemoryPlugin;
 import io.trino.testing.AbstractTestQueryFramework;
@@ -45,15 +47,17 @@ public void reloadCatalog() {
   @Override
   protected QueryRunner createQueryRunner() throws Exception {
     server = closeAfterClass(new GravitinoMockServer());
-    GravitinoPlugin.gravitinoClient = server.createGravitinoClient();
+    GravitinoAdminClient gravitinoClient = server.createGravitinoClient();
 
     Session session = testSessionBuilder().setCatalog("gravitino").build();
     QueryRunner queryRunner = null;
     try {
       // queryRunner = LocalQueryRunner.builder(session).build();
       queryRunner = DistributedQueryRunner.builder(session).setNodeCount(1).build();
 
-      queryRunner.installPlugin(new GravitinoPlugin());
+      TestGravitinoPlugin gravitinoPlugin = new TestGravitinoPlugin();
+      gravitinoPlugin.setGravitinoClient(gravitinoClient);
+      queryRunner.installPlugin(gravitinoPlugin);
       queryRunner.installPlugin(new MemoryPlugin());
 
       {
@@ -71,10 +75,14 @@ protected QueryRunner createQueryRunner() throws Exception {
         properties.put("gravitino.uri", "http://127.0.0.1:8090");
         queryRunner.createCatalog("test1", "gravitino", properties);
       }
-      server.setCatalogConnectorManager(GravitinoPlugin.catalogConnectorManager);
+
+      CatalogConnectorManager catalogConnectorManager =
+          gravitinoPlugin.getCatalogConnectorManager();
+      catalogConnectorManager.setGravitinoClient(gravitinoClient);
+      server.setCatalogConnectorManager(catalogConnectorManager);
       // Wait for the catalog to be created. Wait for at least 30 seconds.
       int max_tries = 35;
-      while (GravitinoPlugin.catalogConnectorManager.getCatalogs().isEmpty() && max_tries > 0) {
+      while (catalogConnectorManager.getCatalogs().isEmpty() && max_tries > 0) {
         Thread.sleep(1000);
         max_tries--;
       }
@@ -254,8 +262,8 @@ public void testCreateCatalog() throws Exception {
     assertThat(computeActual("show catalogs").getOnlyColumnAsSet()).doesNotContain("test.memory1");
 
     // test metalake named test1. the connnector name is test1
-    GravitinoPlugin.gravitinoClient.createMetalake(
-        NameIdentifier.ofMetalake("test1"), "", Collections.emptyMap());
+    GravitinoAdminClient gravitinoClient = server.createGravitinoClient();
+    gravitinoClient.createMetalake(NameIdentifier.ofMetalake("test1"), "", Collections.emptyMap());
 
     assertUpdate("call test1.system.create_catalog('memory1', 'memory', Map())");
     assertThat(computeActual("show catalogs").getOnlyColumnAsSet()).contains("test1.memory1");

trino-connector/src/test/java/com/datastrato/gravitino/trino/connector/TestGravitinoConnectorFactory.java

@@ -0,0 +1,21 @@
+/*
+ * Copyright 2024 Datastrato Pvt Ltd.
+ * This software is licensed under the Apache License version 2.
+ */
+package com.datastrato.gravitino.trino.connector;
+
+import com.datastrato.gravitino.client.GravitinoAdminClient;
+import java.util.function.Supplier;
+
+public class TestGravitinoConnectorFactory extends GravitinoConnectorFactory {
+  private GravitinoAdminClient gravitinoClient;
+
+  public void setGravitinoClient(GravitinoAdminClient client) {
+    this.gravitinoClient = client;
+  }
+
+  @Override
+  Supplier<GravitinoAdminClient> clientProvider() {
+    return () -> gravitinoClient;
+  }
+}

trino-connector/src/test/java/com/datastrato/gravitino/trino/connector/TestGravitinoPlugin.java

@@ -0,0 +1,31 @@
+/*
+ * Copyright 2024 Datastrato Pvt Ltd.
+ * This software is licensed under the Apache License version 2.
+ */
+package com.datastrato.gravitino.trino.connector;
+
+import com.datastrato.gravitino.client.GravitinoAdminClient;
+import com.datastrato.gravitino.trino.connector.catalog.CatalogConnectorManager;
+import com.google.common.collect.ImmutableList;
+import io.trino.spi.connector.ConnectorFactory;
+
+public class TestGravitinoPlugin extends GravitinoPlugin {
+  private TestGravitinoConnectorFactory factory;
+
+  private GravitinoAdminClient gravitinoClient;
+
+  @Override
+  public Iterable<ConnectorFactory> getConnectorFactories() {
+    factory = new TestGravitinoConnectorFactory();
+    factory.setGravitinoClient(gravitinoClient);
+    return ImmutableList.of(factory);
+  }
+
+  public void setGravitinoClient(GravitinoAdminClient client) {
+    this.gravitinoClient = client;
+  }
+
+  public CatalogConnectorManager getCatalogConnectorManager() {
+    return factory.getCatalogConnectorManager();
+  }
+}