[GitHub] [hbase] ndimiduk commented on a change in pull request #1811: HBASE-24474 Rename LocalRegion to MasterRegion

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

ndimiduk commented on a change in pull request #1811:
URL: https://github.com/apache/hbase/pull/1811#discussion_r433425854



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
##########
@@ -450,7 +451,7 @@ public void run() {
   private ProcedureStore procedureStore;
 
   // the master local storage to store procedure data, etc.
-  private LocalStore localStore;
+  private MasterRegion masterRegion;

Review comment:
       Good.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/region/MasterRegion.java
##########
@@ -79,14 +79,14 @@
  * Notice that, you can use different root file system and WAL file system. Then the above directory
  * will be on two file systems, the root file system will have the data directory while the WAL
  * filesystem will have the WALs directory. The archived HFile will be moved to the global HFile
- * archived directory with the {@link LocalRegionParams#archivedWalSuffix()} suffix. The archived
+ * archived directory with the {@link MasterRegionParams#archivedWalSuffix()} suffix. The archived
  * WAL will be moved to the global WAL archived directory with the
- * {@link LocalRegionParams#archivedHFileSuffix()} suffix.
+ * {@link MasterRegionParams#archivedHFileSuffix()} suffix.
  */
 @InterfaceAudience.Private
-public final class LocalRegion {
+public final class MasterRegion {

Review comment:
       We still need the wrapper class with delegation? How about have the factory manage creation of the `HRegion` (wiring up the wal, &c) and `HMaster` hold the instance of `HRegion` directly?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
##########
@@ -1563,7 +1564,7 @@ protected void stopServiceThreads() {
   private void createProcedureExecutor() throws IOException {
     MasterProcedureEnv procEnv = new MasterProcedureEnv(this);
     procedureStore =
-      new RegionProcedureStore(this, localStore, new MasterProcedureEnv.FsUtilsLeaseRecovery(this));
+      new RegionProcedureStore(this, masterRegion, new MasterProcedureEnv.FsUtilsLeaseRecovery(this));

Review comment:
       Having everything use a single region has me a little nervous. Seems like it'll make it easy for two unrelated subsystems to step on each others' toes later on -- conflicting row keys, columns, &c. This should be fine for initial work though.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -258,6 +259,14 @@
   public static final String SPECIAL_RECOVERED_EDITS_DIR =
     "hbase.hregion.special.recovered.edits.dir";
 
+  /**
+   * Whether to use {@link MetaCellComparator} even if we are not meta region. Used when creating
+   * master local region.
+   */
+  public static final String USE_META_CELL_COMPARATOR = "hbase.region.use.meta.cell.comparator";

Review comment:
       If this configuration point is specific to master side, should it have `master` in its name?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/region/MasterRegionFactory.java
##########
@@ -89,45 +82,8 @@
   private static final TableDescriptor TABLE_DESC = TableDescriptorBuilder.newBuilder(TABLE_NAME)
     .setColumnFamily(ColumnFamilyDescriptorBuilder.of(PROC_FAMILY)).build();
 
-  private final LocalRegion region;
-
-  private LocalStore(LocalRegion region) {
-    this.region = region;
-  }
-
-  public void update(UpdateLocalRegion action) throws IOException {
-    region.update(action);
-  }
-
-  public Result get(Get get) throws IOException {
-    return region.get(get);
-  }
-
-  public RegionScanner getScanner(Scan scan) throws IOException {
-    return region.getScanner(scan);
-  }
-
-  public void close(boolean abort) {
-    region.close(abort);
-  }
-
-  @VisibleForTesting
-  public FlushResult flush(boolean force) throws IOException {
-    return region.flush(force);
-  }
-
-  @VisibleForTesting
-  public void requestRollAll() {
-    region.requestRollAll();
-  }
-
-  @VisibleForTesting
-  public void waitUntilWalRollFinished() throws InterruptedException {
-    region.waitUntilWalRollFinished();
-  }
-
-  public static LocalStore create(Server server) throws IOException {
-    LocalRegionParams params = new LocalRegionParams().server(server)

Review comment:
       👍 

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -258,6 +259,14 @@
   public static final String SPECIAL_RECOVERED_EDITS_DIR =
     "hbase.hregion.special.recovered.edits.dir";
 
+  /**
+   * Whether to use {@link MetaCellComparator} even if we are not meta region. Used when creating
+   * master local region.
+   */
+  public static final String USE_META_CELL_COMPARATOR = "hbase.region.use.meta.cell.comparator";

Review comment:
       It's a little nit-pick, but I like my configurations to specify components via `.`-separator, and use `_` for component names. So this might be `hbase.master.region.use_meta_cell_comparator`.
   
   Of course, this might be conflicting with other configuration names and uses. Would need to look things over to see what else is using the `"hbase.master"` namespace.




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