HBASE-26144 The HStore.snapshot method is never called in main code (#3533)

Signed-off-by: Yulin Niu <niuyulin@apache.org>

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java

@@ -982,22 +982,8 @@ public class HStore implements Store, HeapSize, StoreConfigInformation,
   }
 
   /**
-   * Snapshot this stores memstore. Call before running
-   * {@link #flushCache(long, MemStoreSnapshot, MonitoredTask, ThroughputController,
-   * FlushLifeCycleTracker)}
-   *  so it has some work to do.
-   */
-  void snapshot() {
-    this.lock.writeLock().lock();
-    try {
-      this.memstore.snapshot();
-    } finally {
-      this.lock.writeLock().unlock();
-    }
-  }
-
-  /**
-   * Write out current snapshot. Presumes {@link #snapshot()} has been called previously.
+   * Write out current snapshot. Presumes {@code StoreFlusherImpl.prepare()} has been called
+   * previously.
    * @param logCacheFlushId flush sequence number
    * @return The path name of the tmp file to which the store was flushed
    * @throws IOException if exception occurs during process
@@ -1240,9 +1226,6 @@ public class HStore implements Store, HeapSize, StoreConfigInformation,
     this.lock.writeLock().lock();
     try {
       this.storeEngine.getStoreFileManager().insertNewFiles(sfs);
-      if (snapshotId > 0) {
-        this.memstore.clearSnapshot(snapshotId);
-      }
     } finally {
       // We need the lock, as long as we are updating the storeFiles
       // or changing the memstore. Let us release it before calling
@@ -1251,6 +1234,13 @@ public class HStore implements Store, HeapSize, StoreConfigInformation,
       // the lock.
       this.lock.writeLock().unlock();
     }
+    // We do not need to call clearSnapshot method inside the write lock.
+    // The clearSnapshot itself is thread safe, which can be called at the same time with other
+    // memstore operations expect snapshot and clearSnapshot. And for these two methods, in HRegion
+    // we can guarantee that there is only one onging flush, so they will be no race.
+    if (snapshotId > 0) {
+      this.memstore.clearSnapshot(snapshotId);
+    }
     // notify to be called here - only in case of flushes
     notifyChangedReadersObservers(sfs);
     if (LOG.isTraceEnabled()) {

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHMobStore.java

@@ -461,11 +461,8 @@ public class TestHMobStore {
 
   /**
    * Flush the memstore
-   * @param storeFilesSize
-   * @throws IOException
    */
   private void flush(int storeFilesSize) throws IOException{
-    this.store.snapshot();
     flushStore(store, id++);
     Assert.assertEquals(storeFilesSize, this.store.getStorefiles().size());
     Assert.assertEquals(0, ((AbstractMemStore)this.store.memstore).getActive().getCellsCount());
@@ -473,9 +470,6 @@ public class TestHMobStore {
 
   /**
    * Flush the memstore
-   * @param store
-   * @param id
-   * @throws IOException
    */
   private static void flushStore(HMobStore store, long id) throws IOException {
     StoreFlushContext storeFlushCtx = store.createFlushContext(id, FlushLifeCycleTracker.DUMMY);

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java

@@ -629,8 +629,7 @@ public class TestHStore {
     assertCheck();
   }
 
-  private void flush(int storeFilessize) throws IOException{
-    this.store.snapshot();
+  private void flush(int storeFilessize) throws IOException {
     flushStore(store, id++);
     assertEquals(storeFilessize, this.store.getStorefiles().size());
     assertEquals(0, ((AbstractMemStore)this.store.memstore).getActive().getCellsCount());
@@ -801,7 +800,6 @@ public class TestHStore {
 
   /**
    * Test to ensure correctness when using Stores with multiple timestamps
-   * @throws IOException
    */
   @Test
   public void testMultipleTimestamps() throws IOException {
@@ -816,7 +814,6 @@ public class TestHStore {
       this.store.add(kv, null);
     }
 
-    this.store.snapshot();
     flushStore(store, id++);
 
     List<Cell> kvList2 = getKeyValueSet(timestamps2,numRows, qf1, family);