[GitHub] [hbase] Apache9 commented on a diff in pull request #4457: HBASE-27028 Add a shell command for flushing master local region

2022-06-09 Thread GitBox 


Apache9 commented on code in PR #4457:
URL: https://github.com/apache/hbase/pull/4457#discussion_r893675901


##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/region/MasterRegionFlusherAndCompactor.java:
##
@@ -263,6 +263,14 @@ void requestFlush() {
 }
   }
 
+  void resetChangesAfterLastFlush() {
+changesAfterLastFlush.set(0);
+  }
+
+  void resetLastFlushTime() {

Review Comment:
   Should name it recordLastFlushTime?



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] Apache9 commented on a diff in pull request #4457: HBASE-27028 Add a shell command for flushing master local region

2022-06-06 Thread GitBox 


Apache9 commented on code in PR #4457:
URL: https://github.com/apache/hbase/pull/4457#discussion_r890684092


##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##
@@ -457,6 +457,9 @@ public class HMaster extends 
HBaseServerBase implements Maste
   public static final String WARMUP_BEFORE_MOVE = 
"hbase.master.warmup.before.move";
   private static final boolean DEFAULT_WARMUP_BEFORE_MOVE = true;
 
+  // Only for testing
+  private boolean isLocalRegionFlushed = false;

Review Comment:
   Is it possible to not adding a flag which is only used to verify test 
result? I think you can add a CP method for the flushMasterRegion call, and use 
attach a CP to verify that whether the master is called.



##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/region/MasterRegion.java:
##
@@ -155,7 +158,16 @@ public RegionScanner getRegionScanner(Scan scan) throws 
IOException {
   }
 
   public FlushResult flush(boolean force) throws IOException {
-return region.flush(force);
+flusherAndCompactor.resetChangesAfterLastFlush();

Review Comment:
   We do not need to record the lastFlushTime here?



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] Apache9 commented on a diff in pull request #4457: HBASE-27028 Add a shell command for flushing master local region

2022-06-01 Thread GitBox 


Apache9 commented on code in PR #4457:
URL: https://github.com/apache/hbase/pull/4457#discussion_r886353048


##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/region/MasterRegion.java:
##
@@ -155,7 +158,16 @@ public RegionScanner getRegionScanner(Scan scan) throws 
IOException {
   }
 
   public FlushResult flush(boolean force) throws IOException {
-return region.flush(force);
+flusherAndCompactor.resetChangesAfterLastFlush();

Review Comment:
   I mean we have reset logic in MasterRegionFlusherAndCompactor, before 
calling this flush method. Now we move the reset logic into this method, then 
we should remove the reset logic in MasterRegionFlusherAndCompactor?



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] Apache9 commented on a diff in pull request #4457: HBASE-27028 Add a shell command for flushing master local region

2022-05-31 Thread GitBox 


Apache9 commented on code in PR #4457:
URL: https://github.com/apache/hbase/pull/4457#discussion_r885637922


##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/region/MasterRegion.java:
##
@@ -155,7 +158,16 @@ public RegionScanner getRegionScanner(Scan scan) throws 
IOException {
   }
 
   public FlushResult flush(boolean force) throws IOException {
-return region.flush(force);
+flusherAndCompactor.resetChangesAfterLastFlush();

Review Comment:
   We add a line, but where do we reset this in tha past? We do not need to 
remove the original one?



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] Apache9 commented on a diff in pull request #4457: HBASE-27028 Add a shell command for flushing master local region

2022-05-31 Thread GitBox 


Apache9 commented on code in PR #4457:
URL: https://github.com/apache/hbase/pull/4457#discussion_r885261945


##
hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java:
##
@@ -2539,4 +2539,9 @@ default BalanceResponse balanceRSGroup(String groupName) 
throws IOException {
*/
   List getLogEntries(Set serverNames, String logType, 
ServerType serverType,
 int limit, Map filterParams) throws IOException;
+
+  /**
+   * Force request flush master local region
+   */
+  void forceRequestFlushMasterStore() throws IOException;

Review Comment:
   Then we could introduce a method in MasterRegionFlusherAndCompactor? Just 
reset changesAfterLastFlush after calling flush?
   
   Anyway, what I mean is we should try our best to align the behavior in our 
Admin interface.



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] Apache9 commented on a diff in pull request #4457: HBASE-27028 Add a shell command for flushing master local region

2022-05-30 Thread GitBox 


Apache9 commented on code in PR #4457:
URL: https://github.com/apache/hbase/pull/4457#discussion_r885194861


##
hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java:
##
@@ -2539,4 +2539,9 @@ default BalanceResponse balanceRSGroup(String groupName) 
throws IOException {
*/
   List getLogEntries(Set serverNames, String logType, 
ServerType serverType,
 int limit, Map filterParams) throws IOException;
+
+  /**
+   * Force request flush master local region
+   */
+  void forceRequestFlushMasterStore() throws IOException;

Review Comment:
   I mean add a force flag... Not add a force to the method name...
   
   Checked the code, we do not have this flag in the Admin interface, and at rs 
side, it will always use true when calling region.flush...
   
   So sorry, let's keep align with other methods in this interface, just call 
it flushMasterStore, and at master side, we call region.flush(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.

To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] Apache9 commented on a diff in pull request #4457: HBASE-27028 Add a shell command for flushing master local region

2022-05-27 Thread GitBox 


Apache9 commented on code in PR #4457:
URL: https://github.com/apache/hbase/pull/4457#discussion_r884051841


##
hbase-server/src/main/java/org/apache/hadoop/hbase/master/region/MasterRegion.java:
##
@@ -154,6 +154,10 @@ public RegionScanner getRegionScanner(Scan scan) throws 
IOException {
 return region.getScanner(scan);
   }
 
+  public void rpcRequestFlush() {

Review Comment:
   Just call it requestFlush?



##
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java:
##
@@ -1078,4 +1078,9 @@ public List getLogEntries(Set 
serverNames, String logType,
 ServerType serverType, int limit, Map filterParams) throws 
IOException {
 return get(admin.getLogEntries(serverNames, logType, serverType, limit, 
filterParams));
   }
+
+  @Override
+  public void flushMasterStore() throws IOException {

Review Comment:
   Do you think we need to add a force flag here?



-- 
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.

To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org