[GitHub] [hadoop] omalley commented on a diff in pull request #4127: HDFS-16767. RBF: Support observer node from Router-Based Federation

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

omalley commented on code in PR #4127:
URL: https://github.com/apache/hadoop/pull/4127#discussion_r969937548


##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java:
##########
@@ -200,6 +207,17 @@ public RouterRpcClient(Configuration conf, Router router,
         failoverSleepBaseMillis, failoverSleepMaxMillis);
     String[] ipProxyUsers = conf.getStrings(DFS_NAMENODE_IP_PROXY_USERS);
     this.enableProxyUser = ipProxyUsers != null && ipProxyUsers.length > 0;
+    this.observerReadEnabled = conf.getBoolean(
+        RBFConfigKeys.DFS_ROUTER_OBSERVER_READ_ENABLE,
+        RBFConfigKeys.DFS_ROUTER_OBSERVER_READ_ENABLE_DEFAULT);
+    Map<String, String> observerReadOverrides = conf
+        .getPropsWithPrefix(RBFConfigKeys.DFS_ROUTER_OBSERVER_READ_ENABLE + ".");

Review Comment:
   I'd rather have this be a property that inverts the default with a name like FEDERATION_ROUTER_PREFIX + ".observer.read.overrides" that is a list of namespaces that are not the default. It will be easier to configure with a single property and it can be in RBFConfigKeys.



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/resolver/MembershipNamenodeResolver.java:
##########
@@ -73,8 +74,13 @@
   /** Parent router ID. */
   private String routerId;
 
-  /** Cached lookup of NN for nameservice. Invalidated on cache refresh. */
+  /** Cached lookup of NN for nameservice with active state ranked first.

Review Comment:
   Rather than having a second map, I'd suggest adding the useObserver as part of the key and making it: Map<Pair<String,Boolean>,List<...>>



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcServer.java:
##########
@@ -337,18 +337,17 @@ public RouterRpcServer(Configuration conf, Router router,
         .build();
 
     // Add all the RPC protocols that the Router implements
-    DFSUtil.addPBProtocol(
-        conf, NamenodeProtocolPB.class, nnPbService, this.rpcServer);
-    DFSUtil.addPBProtocol(conf, RefreshUserMappingsProtocolPB.class,
+    DFSUtil.addPBProtocol(this.conf, NamenodeProtocolPB.class, nnPbService, this.rpcServer);
+    DFSUtil.addPBProtocol(this.conf, RefreshUserMappingsProtocolPB.class,
         refreshUserMappingService, this.rpcServer);
-    DFSUtil.addPBProtocol(conf, GetUserMappingsProtocolPB.class,
+    DFSUtil.addPBProtocol(this.conf, GetUserMappingsProtocolPB.class,

Review Comment:
   Do we need to make the change from "conf" to "this.conf"?



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java:
##########
@@ -462,7 +481,8 @@ private RetryDecision shouldRetry(final IOException ioe, final int retryCount,
   public Object invokeMethod(
       final UserGroupInformation ugi,
       final List<? extends FederationNamenodeContext> namenodes,
-      final Class<?> protocol, final Method method, final Object... params)
+      final Class<?> protocol, final Method method, boolean skipObserver,

Review Comment:
   It's a nit, but I'd move skipObserver up next to the namenodes. At the higher level we use useObserver and negate when calling here. We should probably be consistent.



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RBFConfigKeys.java:
##########
@@ -191,6 +191,10 @@ public class RBFConfigKeys extends CommonConfigurationKeysPublic {
       FEDERATION_STORE_PREFIX + "enable";
   public static final boolean DFS_ROUTER_STORE_ENABLE_DEFAULT = true;
 
+  public static final String DFS_ROUTER_OBSERVER_READ_ENABLE =

Review Comment:
   This is really the default of whether observers are enabled, so I'd propose:
   DFS_ROUTER_OBSERVER_READ = 
   FEDERATION_ROUTER_PREFIX + ".observer.read.default"



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org