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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
