[GitHub] [hadoop] omalley commented on a diff in pull request #4311: HDFS-13522: IPC changes to support observer reads through routers.

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

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