[GitHub] [hbase] Apache9 commented on a diff in pull request #4868: HBASE-27474 Evict blocks on split/merge; Avoid caching reference/hlinks if compaction is enabled

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

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


##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java:
##########
@@ -145,6 +148,12 @@ private void setInitialAndLastState() {
 
   protected TransitRegionStateProcedure(MasterProcedureEnv env, RegionInfo hri,
     ServerName assignCandidate, boolean forceNewPlan, TransitionType type) {
+    this(env, hri, assignCandidate, forceNewPlan, type, Optional.empty());
+  }
+
+  protected TransitRegionStateProcedure(MasterProcedureEnv env, RegionInfo hri,
+    ServerName assignCandidate, boolean forceNewPlan, TransitionType type,
+    Optional<Boolean> evictCache) {

Review Comment:
   Do not use Optional as parameter.



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java:
##########
@@ -45,14 +46,17 @@ public class CloseRegionProcedure extends RegionRemoteProcedureBase {
   // wrong(but do not make it wrong intentionally). The client can handle this error.
   private ServerName assignCandidate;
 
+  private Optional<Boolean> evictCache;
+
   public CloseRegionProcedure() {
     super();
   }
 
   public CloseRegionProcedure(TransitRegionStateProcedure parent, RegionInfo region,
-    ServerName targetServer, ServerName assignCandidate) {
+    ServerName targetServer, ServerName assignCandidate, Optional<Boolean> evictCache) {

Review Comment:
   It is an anti pattern to pass in an Optional. Just introduce another constructor to accept a boolean.



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java:
##########
@@ -277,6 +280,10 @@ Throwable getException() {
   /** Returns Instance of HRegion if successful open else null. */
   private HRegion openRegion() {
     HRegion region = null;
+    boolean compactionEnabled =
+      ((HRegionServer) server).getCompactSplitThread().isCompactionsEnabled();
+    this.server.getConfiguration().setBoolean(HBASE_REGION_SERVER_ENABLE_COMPACTION,

Review Comment:
   Why change the global configuration here?



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java:
##########
@@ -120,6 +121,8 @@
 
   private RegionRemoteProcedureBase remoteProc;
 
+  private Optional<Boolean> evictCache;

Review Comment:
   Serialize this field?



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/UnassignRegionHandler.java:
##########
@@ -144,14 +158,14 @@ protected void handleException(Throwable t) {
   }
 
   public static UnassignRegionHandler create(HRegionServer server, String encodedName,
-    long closeProcId, boolean abort, @Nullable ServerName destination) {
+    long closeProcId, boolean abort, @Nullable ServerName destination, boolean evictCache) {

Review Comment:
   This is not optional?



##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java:
##########
@@ -45,14 +46,17 @@ public class CloseRegionProcedure extends RegionRemoteProcedureBase {
   // wrong(but do not make it wrong intentionally). The client can handle this error.
   private ServerName assignCandidate;
 
+  private Optional<Boolean> evictCache;

Review Comment:
   We do not need to serialize this field?



-- 
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-unsubscribe@hbase.apache.org

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