[jira] [Commented] (RATIS-141) In RaftClientProtocolService, the assumption of consecutive callId is invalid
[
https://issues.apache.org/jira/browse/RATIS-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16248261#comment-16248261
]
Tsz Wo Nicholas Sze commented on RATIS-141:
---
It seems that we need a new id, namely streamId, for the calls in a stream.
> In RaftClientProtocolService, the assumption of consecutive callId is invalid
> -
>
> Key: RATIS-141
> URL: https://issues.apache.org/jira/browse/RATIS-141
> Project: Ratis
> Issue Type: Bug
>Reporter: Tsz Wo Nicholas Sze
>
> {code}
> //RaftClientProtocolService.AppendRequestStreamObserver.onNext(..)
> // we assume the callId is consecutive for a stream RPC call
> final PendingAppend pendingForReply = pendingList.get(
> (int) (replySeq - headSeqNum));
> {code}
> Call id is used for different kinds of calls (e.g. getInfo) so that it may
> not be consecutive.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (RATIS-141) In RaftClientProtocolService, the assumption of consecutive callId is invalid
[
https://issues.apache.org/jira/browse/RATIS-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16250022#comment-16250022
]
Tsz Wo Nicholas Sze commented on RATIS-141:
---
It actually is better to call it streamSeqNum. How about we add the
streamSeqNum field to RaftRpcRequestProto and RaftRpcReplyProto? Then, all the
calls can be sent via a stream.
> In RaftClientProtocolService, the assumption of consecutive callId is invalid
> -
>
> Key: RATIS-141
> URL: https://issues.apache.org/jira/browse/RATIS-141
> Project: Ratis
> Issue Type: Bug
>Reporter: Tsz Wo Nicholas Sze
>
> {code}
> //RaftClientProtocolService.AppendRequestStreamObserver.onNext(..)
> // we assume the callId is consecutive for a stream RPC call
> final PendingAppend pendingForReply = pendingList.get(
> (int) (replySeq - headSeqNum));
> {code}
> Call id is used for different kinds of calls (e.g. getInfo) so that it may
> not be consecutive.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (RATIS-141) In RaftClientProtocolService, the assumption of consecutive callId is invalid
[
https://issues.apache.org/jira/browse/RATIS-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16250460#comment-16250460
]
Jing Zhao commented on RATIS-141:
-
Yeah, sounds good to me. In the meanwhile, let me check why we made this
assumption in the very beginning.
> In RaftClientProtocolService, the assumption of consecutive callId is invalid
> -
>
> Key: RATIS-141
> URL: https://issues.apache.org/jira/browse/RATIS-141
> Project: Ratis
> Issue Type: Bug
>Reporter: Tsz Wo Nicholas Sze
>
> {code}
> //RaftClientProtocolService.AppendRequestStreamObserver.onNext(..)
> // we assume the callId is consecutive for a stream RPC call
> final PendingAppend pendingForReply = pendingList.get(
> (int) (replySeq - headSeqNum));
> {code}
> Call id is used for different kinds of calls (e.g. getInfo) so that it may
> not be consecutive.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (RATIS-141) In RaftClientProtocolService, the assumption of consecutive callId is invalid
[
https://issues.apache.org/jira/browse/RATIS-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16257899#comment-16257899
]
Tsz Wo Nicholas Sze commented on RATIS-141:
---
r141_20171117.patch: add streamSeqNum to RaftRpcRequestProto.
RaftRpcReplyProto actually does not need streamSeqNum since the callId is
already unique.
Will test the patch more.
> In RaftClientProtocolService, the assumption of consecutive callId is invalid
> -
>
> Key: RATIS-141
> URL: https://issues.apache.org/jira/browse/RATIS-141
> Project: Ratis
> Issue Type: Bug
>Reporter: Tsz Wo Nicholas Sze
>Assignee: Tsz Wo Nicholas Sze
> Attachments: r141_20171117.patch
>
>
> {code}
> //RaftClientProtocolService.AppendRequestStreamObserver.onNext(..)
> // we assume the callId is consecutive for a stream RPC call
> final PendingAppend pendingForReply = pendingList.get(
> (int) (replySeq - headSeqNum));
> {code}
> Call id is used for different kinds of calls (e.g. getInfo) so that it may
> not be consecutive.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (RATIS-141) In RaftClientProtocolService, the assumption of consecutive callId is invalid
[
https://issues.apache.org/jira/browse/RATIS-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16258695#comment-16258695
]
Tsz Wo Nicholas Sze commented on RATIS-141:
---
TestRaftStream.testSimpleWrite may fail WITHOUT any patch.
{code}
Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 8.28 sec <<<
FAILURE! - in org.apache.ratis.grpc.TestRaftStream
testSimpleWrite(org.apache.ratis.grpc.TestRaftStream) Time elapsed: 7.479 sec
<<< FAILURE!
java.lang.AssertionError: expected:<500> but was:<350>
at
org.apache.ratis.grpc.TestRaftStream.checkLog(TestRaftStream.java:106)
at
org.apache.ratis.grpc.TestRaftStream.testSimpleWrite(TestRaftStream.java:100)
Results :
Failed tests:
TestRaftStream.testSimpleWrite:100->checkLog:106 expected:<500> but was:<350>
{code}
I will see if it is easy to fix.
> In RaftClientProtocolService, the assumption of consecutive callId is invalid
> -
>
> Key: RATIS-141
> URL: https://issues.apache.org/jira/browse/RATIS-141
> Project: Ratis
> Issue Type: Bug
>Reporter: Tsz Wo Nicholas Sze
>Assignee: Tsz Wo Nicholas Sze
> Attachments: r141_20171117.patch
>
>
> {code}
> //RaftClientProtocolService.AppendRequestStreamObserver.onNext(..)
> // we assume the callId is consecutive for a stream RPC call
> final PendingAppend pendingForReply = pendingList.get(
> (int) (replySeq - headSeqNum));
> {code}
> Call id is used for different kinds of calls (e.g. getInfo) so that it may
> not be consecutive.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (RATIS-141) In RaftClientProtocolService, the assumption of consecutive callId is invalid
[
https://issues.apache.org/jira/browse/RATIS-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16258714#comment-16258714
]
Tsz Wo Nicholas Sze commented on RATIS-141:
---
It seems easier to reproduce the failure of TestRaftStream if the
AppendStreamer.LOG is turned off.
{code}
+++ b/ratis-grpc/src/test/java/org/apache/ratis/grpc/TestRaftStream.java
@@ -46,7 +46,7 @@ import static org.junit.Assert.fail;
public class TestRaftStream extends BaseTest {
static {
-LogUtils.setLogLevel(AppendStreamer.LOG, Level.ALL);
+//LogUtils.setLogLevel(AppendStreamer.LOG, Level.ALL);
}
{code}
With the script in RATIS-147, run
{code}
./dev-support/run-test-repeatedly.sh TestRaftStream#testSimpleWrite
{code}
> In RaftClientProtocolService, the assumption of consecutive callId is invalid
> -
>
> Key: RATIS-141
> URL: https://issues.apache.org/jira/browse/RATIS-141
> Project: Ratis
> Issue Type: Bug
>Reporter: Tsz Wo Nicholas Sze
>Assignee: Tsz Wo Nicholas Sze
> Attachments: r141_20171117.patch
>
>
> {code}
> //RaftClientProtocolService.AppendRequestStreamObserver.onNext(..)
> // we assume the callId is consecutive for a stream RPC call
> final PendingAppend pendingForReply = pendingList.get(
> (int) (replySeq - headSeqNum));
> {code}
> Call id is used for different kinds of calls (e.g. getInfo) so that it may
> not be consecutive.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (RATIS-141) In RaftClientProtocolService, the assumption of consecutive callId is invalid
[
https://issues.apache.org/jira/browse/RATIS-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16258715#comment-16258715
]
Tsz Wo Nicholas Sze commented on RATIS-141:
---
This is a different failed case
{code}
Running org.apache.ratis.grpc.TestRaftStream
Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 3.57 sec <<<
FAILURE! - in org.apache.ratis.grpc.TestRaftStream
testSimpleWrite(org.apache.ratis.grpc.TestRaftStream) Time elapsed: 3.354 sec
<<< FAILURE!
org.junit.internal.ArrayComparisonFailure: arrays first differed at element
[0]; expected:<63> but was:<-81>
at
org.apache.ratis.grpc.TestRaftStream.checkLog(TestRaftStream.java:114)
at
org.apache.ratis.grpc.TestRaftStream.testSimpleWrite(TestRaftStream.java:100)
Results :
Failed tests:
TestRaftStream.testSimpleWrite:100->checkLog:114 arrays first differed at
element [0]; expected:<63> but was:<-81>
{code}
> In RaftClientProtocolService, the assumption of consecutive callId is invalid
> -
>
> Key: RATIS-141
> URL: https://issues.apache.org/jira/browse/RATIS-141
> Project: Ratis
> Issue Type: Bug
>Reporter: Tsz Wo Nicholas Sze
>Assignee: Tsz Wo Nicholas Sze
> Attachments: r141_20171117.patch
>
>
> {code}
> //RaftClientProtocolService.AppendRequestStreamObserver.onNext(..)
> // we assume the callId is consecutive for a stream RPC call
> final PendingAppend pendingForReply = pendingList.get(
> (int) (replySeq - headSeqNum));
> {code}
> Call id is used for different kinds of calls (e.g. getInfo) so that it may
> not be consecutive.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (RATIS-141) In RaftClientProtocolService, the assumption of consecutive callId is invalid
[
https://issues.apache.org/jira/browse/RATIS-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16258728#comment-16258728
]
Tsz Wo Nicholas Sze commented on RATIS-141:
---
> It seems easier to reproduce the failure of TestRaftStream if the
> AppendStreamer.LOG is turned off.
Due to this reason, it looks like that the bug is in AppendStreamer. Consider
that AppendStreamer is only used in rpc.client.RaftOutputStream and tests.
Let's fix it separately; filed RATIS-149.
> In RaftClientProtocolService, the assumption of consecutive callId is invalid
> -
>
> Key: RATIS-141
> URL: https://issues.apache.org/jira/browse/RATIS-141
> Project: Ratis
> Issue Type: Bug
>Reporter: Tsz Wo Nicholas Sze
>Assignee: Tsz Wo Nicholas Sze
> Attachments: r141_20171117.patch
>
>
> {code}
> //RaftClientProtocolService.AppendRequestStreamObserver.onNext(..)
> // we assume the callId is consecutive for a stream RPC call
> final PendingAppend pendingForReply = pendingList.get(
> (int) (replySeq - headSeqNum));
> {code}
> Call id is used for different kinds of calls (e.g. getInfo) so that it may
> not be consecutive.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (RATIS-141) In RaftClientProtocolService, the assumption of consecutive callId is invalid
[
https://issues.apache.org/jira/browse/RATIS-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16263369#comment-16263369
]
Chen Liang commented on RATIS-141:
--
Thanks [~szetszwo] for working on this! Had an offline discussion with
Nicholas. Seems that we may not really need to add this variable
{{asyncSeqNum}}. And just making {{callIdCounter}} non-static would do the same
job.
> In RaftClientProtocolService, the assumption of consecutive callId is invalid
> -
>
> Key: RATIS-141
> URL: https://issues.apache.org/jira/browse/RATIS-141
> Project: Ratis
> Issue Type: Bug
>Reporter: Tsz Wo Nicholas Sze
>Assignee: Tsz Wo Nicholas Sze
> Attachments: r141_20171117.patch, r141_20171119b.patch,
> r141_20171120.patch
>
>
> {code}
> //RaftClientProtocolService.AppendRequestStreamObserver.onNext(..)
> // we assume the callId is consecutive for a stream RPC call
> final PendingAppend pendingForReply = pendingList.get(
> (int) (replySeq - headSeqNum));
> {code}
> Call id is used for different kinds of calls (e.g. getInfo) so that it may
> not be consecutive.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (RATIS-141) In RaftClientProtocolService, the assumption of consecutive callId is invalid
[
https://issues.apache.org/jira/browse/RATIS-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16263441#comment-16263441
]
Tsz Wo Nicholas Sze commented on RATIS-141:
---
> And just making callIdCounter non-static would do the same job.
That's is true. We could assume that the underlying RPC implementation support
either sync or async calls. (when it support async call, the sync call is
implemented using the async calls.) Then, callId's are consecutive per client.
Let me try if it works in RATIS-140.
> In RaftClientProtocolService, the assumption of consecutive callId is invalid
> -
>
> Key: RATIS-141
> URL: https://issues.apache.org/jira/browse/RATIS-141
> Project: Ratis
> Issue Type: Bug
>Reporter: Tsz Wo Nicholas Sze
>Assignee: Tsz Wo Nicholas Sze
> Attachments: r141_20171117.patch, r141_20171119b.patch,
> r141_20171120.patch
>
>
> {code}
> //RaftClientProtocolService.AppendRequestStreamObserver.onNext(..)
> // we assume the callId is consecutive for a stream RPC call
> final PendingAppend pendingForReply = pendingList.get(
> (int) (replySeq - headSeqNum));
> {code}
> Call id is used for different kinds of calls (e.g. getInfo) so that it may
> not be consecutive.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (RATIS-141) In RaftClientProtocolService, the assumption of consecutive callId is invalid
[
https://issues.apache.org/jira/browse/RATIS-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16264955#comment-16264955
]
Tsz Wo Nicholas Sze commented on RATIS-141:
---
[~vagarychen], looked at the code again and found that
- GrpcClientRpc uses blockingStub or adminBlockingStub to send
ReinitializeRequest, SetConfigurationRequest and ServerInformatonRequest
- It uses asyncStub to send other RaftClientRequest.
Therefore, the callId are not consecutive even if callIdCounter is non-static.
Will upload a new patch.
> In RaftClientProtocolService, the assumption of consecutive callId is invalid
> -
>
> Key: RATIS-141
> URL: https://issues.apache.org/jira/browse/RATIS-141
> Project: Ratis
> Issue Type: Bug
>Reporter: Tsz Wo Nicholas Sze
>Assignee: Tsz Wo Nicholas Sze
> Attachments: r141_20171117.patch, r141_20171119b.patch,
> r141_20171120.patch
>
>
> {code}
> //RaftClientProtocolService.AppendRequestStreamObserver.onNext(..)
> // we assume the callId is consecutive for a stream RPC call
> final PendingAppend pendingForReply = pendingList.get(
> (int) (replySeq - headSeqNum));
> {code}
> Call id is used for different kinds of calls (e.g. getInfo) so that it may
> not be consecutive.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (RATIS-141) In RaftClientProtocolService, the assumption of consecutive callId is invalid
[
https://issues.apache.org/jira/browse/RATIS-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16269707#comment-16269707
]
Chen Liang commented on RATIS-141:
--
+1 on v20171125b patch, I've committed the patch, thanks [~szetszwo] for the
contribution!
> In RaftClientProtocolService, the assumption of consecutive callId is invalid
> -
>
> Key: RATIS-141
> URL: https://issues.apache.org/jira/browse/RATIS-141
> Project: Ratis
> Issue Type: Bug
>Reporter: Tsz Wo Nicholas Sze
>Assignee: Tsz Wo Nicholas Sze
> Attachments: r141_20171117.patch, r141_20171119b.patch,
> r141_20171120.patch, r141_20171124.patch, r141_20171125.patch,
> r141_20171125b.patch
>
>
> {code}
> //RaftClientProtocolService.AppendRequestStreamObserver.onNext(..)
> // we assume the callId is consecutive for a stream RPC call
> final PendingAppend pendingForReply = pendingList.get(
> (int) (replySeq - headSeqNum));
> {code}
> Call id is used for different kinds of calls (e.g. getInfo) so that it may
> not be consecutive.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
