[GitHub] [ratis] szetszwo commented on a diff in pull request #700: RATIS-1557. Linearizable Leader Read-Only Request Implementation

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

szetszwo commented on code in PR #700:
URL: https://github.com/apache/ratis/pull/700#discussion_r936223801


##########
ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java:
##########
@@ -722,6 +722,21 @@ static void setPreVote(RaftProperties properties, boolean enablePreVote) {
     }
   }
 
+  interface ReadOnly {

Review Comment:
   Let's call it "Read" instead of "ReadOnly".  Please move it to right before the Write interface, i.e. line 159.



##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -893,6 +893,14 @@ private CompletableFuture<RaftClientReply> staleReadAsync(RaftClientRequest requ
     return processQueryFuture(stateMachine.queryStale(request.getMessage(), minIndex), request);
   }
 
+  private CompletableFuture<RaftClientReply> readOnlyAsync(RaftClientRequest request) {
+    LeaderState leaderState = role.getLeaderStateNonNull();
+    return leaderState.getReadIndex()

Review Comment:
   Change it to 
   ```
       return role.getLeaderStateNonNull().getReadIndex()
   ```
   
   Why if the server is a Follower?



##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -831,7 +831,7 @@ public CompletableFuture<RaftClientReply> submitClientRequestAsync(
       if (type.is(TypeCase.READ)) {
         // TODO: We might not be the leader anymore by the time this completes.
         // See the RAFT paper section 8 (last part)
-        replyFuture = processQueryFuture(stateMachine.query(request.getMessage()), request);
+        replyFuture = readOnlyAsync(request);

Review Comment:
   This linearizable read feature must be configurable.  Some applications may not want to use it.



##########
ratis-server/src/main/java/org/apache/ratis/server/leader/LogAppenderBase.java:
##########
@@ -53,6 +57,8 @@ public abstract class LogAppenderBase implements LogAppender {
   private final LogAppenderDaemon daemon;
   private final AwaitForSignal eventAwaitForSignal;
 
+  private final BlockingQueue<Consumer<AppendEntriesReplyProto>> watcherList;

Review Comment:
   Why using a BlockingQueue?  It seems that a List is good enough.



##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -422,6 +423,33 @@ CompletableFuture<RaftClientReply> addWatchReqeust(RaftClientRequest request) {
         .exceptionally(e -> exception2RaftClientReply(request, e));
   }
 
+  @Override
+  public CompletableFuture<Long> getReadIndex() {

Review Comment:
   Remove `@Override` and `public`.



##########
ratis-server/src/main/java/org/apache/ratis/server/impl/ReadOnlyRequests.java:
##########
@@ -0,0 +1,173 @@
+/*
+ * 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.ratis.server.impl;
+
+import org.apache.ratis.conf.RaftProperties;
+import org.apache.ratis.proto.RaftProtos;
+import org.apache.ratis.protocol.exceptions.ResourceUnavailableException;
+import org.apache.ratis.server.RaftServerConfigKeys;
+import org.apache.ratis.statemachine.StateMachine;
+import org.apache.ratis.util.JavaUtils;
+import org.apache.ratis.util.TimeDuration;
+import org.apache.ratis.util.TimeoutScheduler;
+import org.apache.ratis.util.Timestamp;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+
+public class ReadOnlyRequests {
+    private static final Logger LOG = LoggerFactory.getLogger(ReadOnlyRequests.class);

Review Comment:
   Please use 2-space indentation for all the code.



##########
ratis-server-api/src/main/java/org/apache/ratis/server/leader/LeaderState.java:
##########
@@ -47,6 +48,10 @@ public String toString() {
   AppendEntriesRequestProto newAppendEntriesRequestProto(FollowerInfo follower,
       List<LogEntryProto> entries, TermIndex previous, long callId);
 
+  /** obtain current read index by broadcasting heartbeats and maintain authority
+   * @return readIndex, or exception if anything wrong */
+  CompletableFuture<Long> getReadIndex();

Review Comment:
   Remove this method from LeaderState.  Declare it in LeaderStateImpl only.



##########
ratis-server-api/src/main/java/org/apache/ratis/server/RaftServerConfigKeys.java:
##########
@@ -722,6 +722,21 @@ static void setPreVote(RaftProperties properties, boolean enablePreVote) {
     }
   }
 
+  interface ReadOnly {
+    String PREFIX = RaftServerConfigKeys.PREFIX
+            + "." + JavaUtils.getClassSimpleName(ReadOnly.class).toLowerCase();
+

Review Comment:
   Add a conf to enable/disable linearizable read.



-- 
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: issues-unsubscribe@ratis.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org