2.2.x Fixing bug : Avoid registering subscriber class multiple times (#…

…1227)

* Fixing bug to avoid registering subscriber class multiple times

* Refactoring for Singleton pattern

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleCloudStorageEventSubscriber.java

@@ -18,19 +18,40 @@
 
 import com.google.api.client.googleapis.json.GoogleJsonResponseException;
 import com.google.cloud.hadoop.util.GcsRequestExecutionEvent;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.eventbus.Subscribe;
+import com.google.common.flogger.GoogleLogger;
 import io.grpc.Status;
 import java.io.IOException;
 import javax.annotation.Nonnull;
 
 /* Stores the subscriber methods corresponding to GoogleCloudStorageEventBus */
 public class GoogleCloudStorageEventSubscriber {
+  private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
   private static GhfsStorageStatistics storageStatistics;
+  private static GoogleCloudStorageEventSubscriber INSTANCE = null;
 
-  public GoogleCloudStorageEventSubscriber(GhfsStorageStatistics storageStatistics) {
+  private GoogleCloudStorageEventSubscriber(@Nonnull GhfsStorageStatistics storageStatistics) {
     this.storageStatistics = storageStatistics;
   }
 
+  /*
+   * Singleton class such that registration of subscriber methods is only once.
+   * */
+  public static synchronized GoogleCloudStorageEventSubscriber getInstance(
+      @Nonnull GhfsStorageStatistics storageStatistics) {
+    if (INSTANCE == null) {
+      logger.atFiner().log("Subscriber class invoked for first time");
+      INSTANCE = new GoogleCloudStorageEventSubscriber(storageStatistics);
+    }
+    return INSTANCE;
+  }
+
+  @VisibleForTesting
+  protected static void reset() {
+    INSTANCE = null;
+  }
+
   /**
    * Updating the required gcs specific statistics based on GoogleJsonResponseException.
    *

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemBase.java

@@ -361,7 +361,8 @@ public GoogleHadoopFileSystemBase() {
       storageStatistics = GhfsStorageStatistics.DUMMY_INSTANCE;
     }
 
-    GoogleCloudStorageEventBus.register(new GoogleCloudStorageEventSubscriber(storageStatistics));
+    GoogleCloudStorageEventBus.register(
+        GoogleCloudStorageEventSubscriber.getInstance(storageStatistics));
   }
 
   /**

gcs/src/test/java/com/google/cloud/hadoop/fs/gcs/GoogleCloudStorageStatisticsTest.java

@@ -50,7 +50,7 @@ public class GoogleCloudStorageStatisticsTest {
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
   private GhfsStorageStatistics storageStatistics = new GhfsStorageStatistics();
   protected GoogleCloudStorageEventSubscriber subscriber =
-      new GoogleCloudStorageEventSubscriber(storageStatistics);
+      GoogleCloudStorageEventSubscriber.getInstance(storageStatistics);
 
   @Before
   public void setUp() throws Exception {
@@ -60,6 +60,7 @@ public void setUp() throws Exception {
   @After
   public void cleanup() throws Exception {
     GoogleCloudStorageEventBus.unregister(subscriber);
+    GoogleCloudStorageEventSubscriber.reset();
   }
 
   private void verifyStatistics(GhfsStorageStatistics expectedStats) {
@@ -79,6 +80,21 @@ private void verifyStatistics(GhfsStorageStatistics expectedStats) {
     assertThat(metricsVerified).isTrue();
   }
 
+  @Test
+  public void test_multiple_register_of_statistics() throws Exception {
+    GoogleCloudStorageEventBus.register(subscriber);
+    GoogleCloudStorageEventBus.register(subscriber);
+    GoogleCloudStorageEventBus.register(
+        GoogleCloudStorageEventSubscriber.getInstance(storageStatistics));
+    GoogleCloudStorageEventBus.register(
+        GoogleCloudStorageEventSubscriber.getInstance(storageStatistics));
+
+    GoogleCloudStorageEventBus.onGcsRequest(new GcsRequestExecutionEvent());
+    GhfsStorageStatistics verifyCounterStats = new GhfsStorageStatistics();
+    verifyCounterStats.incrementCounter(GCS_API_REQUEST_COUNT, 1);
+    verifyStatistics(verifyCounterStats);
+  }
+
   @Test
   public void gcs_requestCounter() throws Exception {
     GoogleCloudStorageEventBus.onGcsRequest(new GcsRequestExecutionEvent());

gcs/src/test/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystemIntegrationTest.java

@@ -58,6 +58,7 @@
 import com.google.cloud.hadoop.gcsio.StatisticTypeEnum;
 import com.google.cloud.hadoop.gcsio.StorageResourceId;
 import com.google.cloud.hadoop.gcsio.testing.InMemoryGoogleCloudStorage;
+import com.google.cloud.hadoop.util.GoogleCloudStorageEventBus;
 import com.google.common.collect.ImmutableList;
 import com.google.common.hash.Hashing;
 import com.google.common.primitives.Ints;
@@ -1270,6 +1271,30 @@ public void testInitializeCompatibleWithHadoopCredentialProvider() throws Except
     // Initialization successful with no exception thrown.
   }
 
+  @Test
+  public void register_subscriber_multiple_time() throws Exception {
+    GoogleHadoopFileSystem myGhfs =
+        createInMemoryGoogleHadoopFileSystem(); // registers the subscriber class first time in
+    // myGhfs1
+    StorageStatistics stats = TestUtils.getStorageStatistics();
+
+    GoogleCloudStorageEventBus.register(
+        GoogleCloudStorageEventSubscriber.getInstance(
+            (GhfsStorageStatistics) stats)); // registers the same subscriber class second time
+
+    assertThat(getMetricValue(stats, INVOCATION_CREATE)).isEqualTo(0);
+    assertThat(getMetricValue(stats, FILES_CREATED)).isEqualTo(0);
+
+    try (FSDataOutputStream fout = myGhfs.create(new Path("/file1"))) {
+      fout.writeBytes("Test Content");
+    }
+    assertThat(getMetricValue(stats, INVOCATION_CREATE)).isEqualTo(1);
+    assertThat(getMetricValue(stats, FILES_CREATED)).isEqualTo(1);
+    assertThat(myGhfs.delete(new Path("/file1"))).isTrue();
+
+    TestUtils.verifyDurationMetric((GhfsStorageStatistics) stats, INVOCATION_CREATE, 1);
+  }
+
   @Test
   public void create_IOstatistics() throws IOException {
     GoogleHadoopFileSystem myGhfs = createInMemoryGoogleHadoopFileSystem();