[GitHub] [hbase] wchevreuil commented on a change in pull request #2179: HBASE-24694 Support flush a single column family of table

[GitBox <gi...@apache.org>]

wchevreuil commented on a change in pull request #2179:
URL: https://github.com/apache/hbase/pull/2179#discussion_r463507819



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -513,6 +513,15 @@ default void modifyColumnFamily(TableName tableName, ColumnFamilyDescriptor colu
    */
   void flush(TableName tableName) throws IOException;
 
+  /**
+   * Flush a table. Synchronous operation.

Review comment:
       We should explain better how this overloaded version differs from `flush(TableName tableName)`.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/flush/FlushTableSubprocedure.java
##########
@@ -88,11 +95,15 @@ private void flushRegions() throws ForeignException {
       throw new IllegalStateException("Attempting to flush "
           + table + " but we currently have outstanding tasks");
     }
-
+    List<byte[]> families = null;
+    if (family != null) {
+      LOG.debug("Flush regions with specified family:{}", family);

Review comment:
       Nit: It's better to be more specific when debugging: "About to flush family {} on all regions for table {}"

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/flush/RegionServerFlushTableProcedureManager.java
##########
@@ -183,8 +185,17 @@ public Subprocedure buildSubprocedure(String table) {
 
     @Override
     public Subprocedure buildSubprocedure(String name, byte[] data) {
+      String family = null;
+      if (data.length > 0) {

Review comment:
       Will it always be non null even when we don't specify any family?

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java
##########
@@ -302,6 +302,13 @@
    */
   CompletableFuture<Void> flush(TableName tableName);
 
+  /**
+   * Flush a table.

Review comment:
       We should explain better how this overloaded version differs from flush(TableName tableName).

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/flush/FlushTableSubprocedure.java
##########
@@ -65,7 +72,7 @@ public Void call() throws Exception {
       region.startRegionOperation();
       try {
         LOG.debug("Flush region " + region.toString() + " started...");
-        region.flush(true);
+        region.flushcache(families, false, FlushLifeCycleTracker.DUMMY);

Review comment:
       Can this affect the other versions that don't specify any families, like `flush(TableName tableName)`? Because `region.flush(true)`, internally, loads all families before delegating to `flushcache(List<byte[]> families, boolean writeFlushRequestWalMarker, FlushLifeCycleTracker tracker)`, but it doesn't seem we are doing this now.




----------------------------------------------------------------
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:
users@infra.apache.org