saintstack commented on a change in pull request #1737:
URL: https://github.com/apache/hbase/pull/1737#discussion_r445307380
##########
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:
Oh, I see it in next line. Let me resolve.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -2400,6 +2405,7 @@ public FlushResult flush(boolean force) throws
IOException {
* <p>This method may block for some time, so it should not be called from a
* time-sensitive thread.
* @param forceFlushAllStores whether we want to flush all stores
+ * @param families stores of region to flush.
Review comment:
If null is passed, does that mean all families?
##########
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:
Did you add comment to clarify what is going on here? I don't see it.
Maybe it is elsewhere?
##########
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:
The flag is set when we are in a critical condition -- the WAL count is
in excess of our WAL limit. The flag's intent IIRC is that we flush all stores
regardless of what determination is made at flush time as to which stores are
in need of flush or not; the old edit may actually be hanging out in a store
that is small and not in need of flush normally or in accordance w/ some flush
policy. The flag says 'force' the flus. My understanding is that this is a
FlushRequest usually but the flag changes the request to a demand.
Here you are passing a set of stores. Will these be flushed regardless when
we go to flush? The flush policy won't prevent one or two of these stores being
flushed? At a minimum I'd think this flag would change meaning from 'force
flush all stores' to 'force flush these passed stores' if we are being passed
the families whos flush will allow us clear WALs, or if null families, 'force
flush all stores'.
If the 'force' of a flush is not needed, then we should purge this flag
altogether as it confuses?
I'm unsure of what you intend here. This is an important trigger and you
seem to be undoing it. Pardon me if I'm not following properly.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -2400,6 +2405,7 @@ public FlushResult flush(boolean force) throws
IOException {
* <p>This method may block for some time, so it should not be called from a
* time-sensitive thread.
* @param forceFlushAllStores whether we want to flush all stores
+ * @param families stores of region to flush.
Review comment:
Yeah, looks like it.
##########
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:
Looking around, this is the only time forceFlushAllStores is set to true
-- when we have too many WALs and we need to clear the old ones out.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -2400,6 +2405,7 @@ public FlushResult flush(boolean force) throws
IOException {
* <p>This method may block for some time, so it should not be called from a
* time-sensitive thread.
* @param forceFlushAllStores whether we want to flush all stores
+ * @param families stores of region to flush.
Review comment:
So all families in a Region if 'families' is null or if
forceFlushAllStores is true?
----------------------------------------------------------------
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:
[email protected]
