saintstack commented on a change in pull request #1737: URL: https://github.com/apache/hbase/pull/1737#discussion_r436450913
########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/FlushPolicy.java ########## @@ -46,4 +49,16 @@ protected void configureForRegion(HRegion region) { */ public abstract Collection<HStore> selectStoresToFlush(); + /** + * select stores which matches the specified families + * + * @return the stores need to be flushed. + */ + public Collection<HStore> selectStoresToFlush(Map<byte[], HStore> stores, List<byte[]> families) { Review comment: Could be static? ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/FlushRequester.java ########## @@ -36,6 +38,18 @@ */ boolean requestFlush(HRegion region, boolean forceFlushAllStores, FlushLifeCycleTracker tracker); + /** + * Tell the listener the cache needs to be flushed. + * + * @param region the Region requesting the cache flush + * @param forceFlushAllStores whether we want to flush all stores. e.g., when request from log Review comment: Yeah, if this is true, families are ignored. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractWALRoller.java ########## @@ -180,18 +181,18 @@ public void run() { WAL wal = entry.getKey(); // reset the flag in front to avoid missing roll request before we return from rollWriter. walNeedsRoll.put(wal, Boolean.FALSE); - byte[][] regionsToFlush = null; + Map<byte[], List<byte[]>> regionsToFlush = null; try { // Force the roll if the logroll.period is elapsed or if a roll was requested. - // The returned value is an array of actual region names. + // The returned value is an collection of actual region and family names. regionsToFlush = wal.rollWriter(periodic || entry.getValue().booleanValue()); } catch (WALClosedException e) { LOG.warn("WAL has been closed. Skipping rolling of writer and just remove it", e); iter.remove(); } if (regionsToFlush != null) { - for (byte[] r : regionsToFlush) { - scheduleFlush(Bytes.toString(r)); + for (Map.Entry<byte[], List<byte[]>> r : regionsToFlush.entrySet()) { Review comment: I do like this change in the return type from Region to Region Stores. I am wary that we are changing the default behavior. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/FlushRequester.java ########## @@ -36,6 +38,18 @@ */ boolean requestFlush(HRegion region, boolean forceFlushAllStores, FlushLifeCycleTracker tracker); + /** + * Tell the listener the cache needs to be flushed. + * + * @param region the Region requesting the cache flush + * @param forceFlushAllStores whether we want to flush all stores. e.g., when request from log Review comment: If this is case, maybe say so in the javadoc? ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/FlushRequester.java ########## @@ -36,6 +38,18 @@ */ boolean requestFlush(HRegion region, boolean forceFlushAllStores, FlushLifeCycleTracker tracker); + /** + * Tell the listener the cache needs to be flushed. + * + * @param region the Region requesting the cache flush + * @param forceFlushAllStores whether we want to flush all stores. e.g., when request from log Review comment: If this true, does it mean flush all familes even if only a subset passed in <code>families</code>? ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/LogRoller.java ########## @@ -58,8 +60,8 @@ protected void scheduleFlush(String encodedRegionName) { encodedRegionName, r); return; } - // force flushing all stores to clean old logs - requester.requestFlush(r, true, FlushLifeCycleTracker.DUMMY); + // force flushing specified stores to clean old logs + requester.requestFlush(r, false, families, FlushLifeCycleTracker.DUMMY); Review comment: We are in here because a force roll was requested. You change the true to false. Isn't that changing behavior at least for the default flush all stores case? ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java ########## @@ -2457,8 +2463,13 @@ public FlushResultImpl flushcache(boolean forceFlushAllStores, boolean writeFlus } try { - Collection<HStore> specificStoresToFlush = + Collection<HStore> specificStoresToFlush = null; + if (!forceFlushAllStores && families != null) { + specificStoresToFlush = flushPolicy.selectStoresToFlush(stores, families); Review comment: So, looking at the flushPolicy implementations we currently have, I see two: FlushAllStoresPolicy and FlushLargeStoresPolicy Given the current implementations, passing a subset of families doesn't seem appropriate at least for the FlushAllStoresPolicy -- if we flush some families only, then we are not flushing AllStores -- and perhaps for the FlushLargeStoresPolicy since we are not flushing the large but the passed families. Do you need to create a new flush policy? One that does the subset of families passed? If user configures the FlushAllStoresPolicy and instead we only flush a subset, then they will be confused. Why do we pass in stores into selectStoresToFlush? We do not need the list of stores in the flushAllStoresPolicy. It has access to 'this' so can figure what Stores are in the Region. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org