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

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

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


##########
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();
+      }
+      if (alignmentContext instanceof ClientGSIContext) {

Review Comment:
   I might lack context, so we will disable observer read only when alignmentContext is null and the observer read is set to false. Can you probably explain why we we care to check alignmentContext not null first? If it's originally not null, what does that mean? Thank you. 



##########
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/ClientGSIContext.java:
##########
@@ -40,6 +40,14 @@ public class ClientGSIContext implements AlignmentContext {
   private final LongAccumulator lastSeenStateId =
       new LongAccumulator(Math::max, Long.MIN_VALUE);
 
+  public void disableObserverRead() {
+    if(lastSeenStateId.get() > -1L) {
+      throw new IllegalStateException(
+          "Can't disable observer read after communicate.");
+    }
+    lastSeenStateId.accumulate(-1L);

Review Comment:
   nitpick: Is -1L reserved only for disabled observer read situation or will it be used for other (future) cases? If we expect future cases use -2L, -3L, etc, will it be helpful to add a comment/doc to explicitly state -1L is reserved for disabled observer read situation? 



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