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

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

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


##########
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:
   Even when alignment context is not equal to null, we still disable observer reads in this if condition. The not null condition occurs when the ObserverReadProxyProvider is being used.



##########
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:
   I do not expect available use cases but I can document that this is a reserved value for disabling router reads.



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterStateIdContext.java:
##########
@@ -0,0 +1,87 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdfs.server.federation.router;
+
+import java.lang.reflect.Method;
+import java.util.HashSet;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.hdfs.protocol.ClientProtocol;
+import org.apache.hadoop.hdfs.server.namenode.ha.ReadOnly;
+import org.apache.hadoop.ipc.AlignmentContext;
+import org.apache.hadoop.ipc.RetriableException;
+import org.apache.hadoop.ipc.protobuf.RpcHeaderProtos.RpcRequestHeaderProto;
+import org.apache.hadoop.ipc.protobuf.RpcHeaderProtos.RpcResponseHeaderProto;
+
+/**
+ * This is the router implementation responsible for passing
+ * client state id to next level.
+ */
+@InterfaceAudience.Private
+@InterfaceStability.Evolving
+class RouterStateIdContext implements AlignmentContext {

Review Comment:
   This means the client's header state ID accessible in the router. But yes, the value is not used in this PR. https://github.com/apache/hadoop/pull/4127 is the one which then uses the value.



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/ConnectionManager.java:
##########
@@ -172,11 +178,12 @@ public void close() {
    * @param ugi User group information.
    * @param nnAddress Namenode address for the connection.
    * @param protocol Protocol for the connection.
+   * @param nsId Nameservice Identify.

Review Comment:
   Fixed



##########
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:
   With the current approach, the router will perform observer reads unless clients explicitly opt out by sending the -1 in the RPC header. Old clients without this change will use observer reads.
   
   If we switch to setting alignment context to null, INT64.MIN (default header stateID value) would become the new special value. We then won't be able to distinguish between when clients are explicitly opting out, versus times when the alignment context is just not set. AlignmentContext is null unless the ObserverReadProxyProvider is configured. 
   
   Another thing to consider is that when communicating with routers, the state ID has no meaning since the routers cover multiple namespaces while the client only stores one state ID. So we'd still need a special value or range to indicate clients care about alignment. 
   
   I think having a value like -1L which is not an expected state ID is better for debugging than say using >=0 because the state ID could then be interpreted as being a particular namenode's state.



##########
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:
   Added a comment. The keys are the nameservice names.



##########
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) {

Review Comment:
   The transation ID on the namenode starts from zero and only goes up. So negative values can be used to indicate error conditions.



##########
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:
   I just realized setting this to false breaks direct reads to the observer namenodes.
   I've add documentation to alert user of this. I'll investigate further.



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