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

[GitBox]

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

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

[GitBox]

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



##
File path: hbase-shell/src/main/ruby/shell/commands/flush.rb
##
@@ -25,10 +25,11 @@ def help
 Flush all regions in passed table or pass a region row to
 flush an individual region or a region server name whose format
 is 'host,port,startcode', to flush all its regions.
-You can also flush a single column family within a region.
+You can also flush a single column family within a table or region.

Review comment:
   `You can also flush a single column family for all regions within a 
table, or for an specific region only.`

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

Review comment:
   Similar to above.

##
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 with specified column family. Synchronous operation.

Review comment:
   Let's not leave any room for confusion: `Flush the specified column 
family stores on all regions of the passed table. This runs as a synchronous 
operation.`





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