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


##########
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/NameNodeProxiesClient.java:
##########
@@ -349,6 +349,18 @@ public static ClientProtocol 
createProxyWithAlignmentContext(
       boolean withRetries, AtomicBoolean fallbackToSimpleAuth,
       AlignmentContext alignmentContext)
       throws IOException {
+    if (!conf.getBoolean(HdfsClientConfigKeys.DFS_OBSERVER_READ_ENABLE,
+        HdfsClientConfigKeys.DFS_OBSERVER_READ_ENABLE_DEFAULT)) {
+      //Disabled observer read
+      if (alignmentContext == null) {
+        alignmentContext = new ClientGSIContext();

Review Comment:
   The using code seems to handle alignment = null as not caring about 
alignment. Therefore, I think all you need to do is assign the variable to null 
to have the same results as disabling it. You'll remove the need for -1 as a 
special value and this code.



##########
hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml:
##########
@@ -6446,4 +6446,11 @@
       frequently than this time, the client will give up waiting.
     </description>
   </property>
+  <property>
+    <name>dfs.observer.read.enable</name>
+    <value>true</value>
+    <description>
+      Enable observer read for client with router

Review Comment:
   This is for all HDFS clients, right? Not just the routers.
   
   I'd propose something like, "Enable the client to use observer NN for read 
operations. It RBF, it enables the routers to use the observer NN."



##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java:
##########
@@ -1091,6 +1091,10 @@ public void setDeferredResponse(Writable response) {
 
     public void setDeferredError(Throwable t) {
     }
+
+    public long getTimestampNanos() {

Review Comment:
   I think changing the visibility to private does most of the good. I'm less 
interested in making all of the other methods in the class use this accessor.



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/ConnectionManager.java:
##########
@@ -73,6 +75,8 @@ public class ConnectionManager {
 
   /** Queue for creating new connections. */
   private final BlockingQueue<ConnectionPool> creatorQueue;
+
+  private final Map<String, AlignmentContext> alignmentContexts;

Review Comment:
   It would be good to give an example of what the key here is. Is the NN 
address canonicalized? 



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

Reply via email to