diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java index 2e1c862f9c2c..c4e34beca406 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java @@ -982,22 +982,8 @@ public Void call() throws IOException { } /** - * 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 @@ private boolean updateStorefiles(List sfs, long snapshotId) throws I 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 @@ private boolean updateStorefiles(List sfs, long snapshotId) throws I // 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()) { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHMobStore.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHMobStore.java index 318cb18c9fff..2d23a37a46f3 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHMobStore.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHMobStore.java @@ -461,11 +461,8 @@ public void testResolve() throws Exception { /** * 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 @@ private void flush(int storeFilesSize) throws IOException{ /** * 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); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java index c8a0b3869eb2..46995d92f8e6 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHStore.java @@ -629,8 +629,7 @@ public void testGet_FromMemStoreAndFiles() throws IOException { 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 @@ private List getKeyValueSet(long[] timestamps, int numRows, /** * Test to ensure correctness when using Stores with multiple timestamps - * @throws IOException */ @Test public void testMultipleTimestamps() throws IOException { @@ -816,7 +814,6 @@ public void testMultipleTimestamps() throws IOException { this.store.add(kv, null); } - this.store.snapshot(); flushStore(store, id++); List kvList2 = getKeyValueSet(timestamps2,numRows, qf1, family);