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
