You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "szetszwo (via GitHub)" <gi...@apache.org> on 2023/09/29 17:43:16 UTC
[GitHub] [ratis] szetszwo opened a new pull request, #930: RATIS-1897. Make TransactionContext available in DataApi.write(..).
szetszwo opened a new pull request, #930:
URL: https://github.com/apache/ratis/pull/930
See RATIS-1897
--
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
Re: [PR] RATIS-1897. Make TransactionContext available in DataApi.write(..). [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #930:
URL: https://github.com/apache/ratis/pull/930#discussion_r1350712290
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java:
##########
@@ -298,7 +308,7 @@ public EntryWithData getEntryWithData(long index) throws RaftLogIOException {
try {
CompletableFuture<ByteString> future = null;
if (stateMachine != null) {
- future = stateMachine.data().read(entry).exceptionally(ex -> {
+ future = stateMachine.data().read(entry, server.getTransactionContext(entry, false)).exceptionally(ex -> {
Review Comment:
You are right that the context should be obtained from LeaderState. Let me update the change.
--
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
Re: [PR] RATIS-1897. Make TransactionContext available in DataApi.write(..). [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #930:
URL: https://github.com/apache/ratis/pull/930#issuecomment-1760306751
@SzyWilliam , thanks a lot for carefully reviewing this!
--
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
Re: [PR] RATIS-1897. Make TransactionContext available in DataApi.write(..). [ratis]
Posted by "SzyWilliam (via GitHub)" <gi...@apache.org>.
SzyWilliam commented on code in PR #930:
URL: https://github.com/apache/ratis/pull/930#discussion_r1346732080
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -88,6 +88,15 @@ default CompletableFuture<?> write(LogEntryProto entry) {
return CompletableFuture.completedFuture(null);
}
+ /**
+ * Write asynchronously the state machine data in the given log entry to this state machine.
+ *
+ * @return a future for the write task
+ */
+ default CompletableFuture<?> write(LogEntryProto entry, TransactionContext context) {
Review Comment:
Shall we add a `@Nullable` annotation here to notify user the `context` will be null when the peer is a follower?
--
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
Re: [PR] RATIS-1897. Make TransactionContext available in DataApi.write(..). [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #930:
URL: https://github.com/apache/ratis/pull/930#discussion_r1347665740
##########
ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java:
##########
@@ -88,6 +88,15 @@ default CompletableFuture<?> write(LogEntryProto entry) {
return CompletableFuture.completedFuture(null);
}
+ /**
+ * Write asynchronously the state machine data in the given log entry to this state machine.
+ *
+ * @return a future for the write task
+ */
+ default CompletableFuture<?> write(LogEntryProto entry, TransactionContext context) {
Review Comment:
That's a good point! Follower currently creates context in `RaftServerImpl.applyLogToStateMachine`. We should create a context instead of passing null and then the context can be reused. Let me think about it
--
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
Re: [PR] RATIS-1897. Make TransactionContext available in DataApi.write(..). [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #930:
URL: https://github.com/apache/ratis/pull/930#issuecomment-1756191739
Forced push for resolving a conflict.
--
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
Re: [PR] RATIS-1897. Make TransactionContext available in DataApi.write(..). [ratis]
Posted by "SzyWilliam (via GitHub)" <gi...@apache.org>.
SzyWilliam commented on code in PR #930:
URL: https://github.com/apache/ratis/pull/930#discussion_r1349533432
##########
ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java:
##########
@@ -298,7 +308,7 @@ public EntryWithData getEntryWithData(long index) throws RaftLogIOException {
try {
CompletableFuture<ByteString> future = null;
if (stateMachine != null) {
- future = stateMachine.data().read(entry).exceptionally(ex -> {
+ future = stateMachine.data().read(entry, server.getTransactionContext(entry, false)).exceptionally(ex -> {
Review Comment:
The `TransactionManager` manages the `TransactionContext` for non-leaders, while `PendingRequests` manages the context for the leader, am I getting it right?
Normally when `DataApi.read` is called, this peer is leader and is going to replicate the full log to non-leaders. Why do we need to call `getTransactionContext` here?
--
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
Re: [PR] RATIS-1897. Make TransactionContext available in DataApi.write(..). [ratis]
Posted by "SzyWilliam (via GitHub)" <gi...@apache.org>.
SzyWilliam merged PR #930:
URL: https://github.com/apache/ratis/pull/930
--
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
Re: [PR] RATIS-1897. Make TransactionContext available in DataApi.write(..). [ratis]
Posted by "SzyWilliam (via GitHub)" <gi...@apache.org>.
SzyWilliam commented on PR #930:
URL: https://github.com/apache/ratis/pull/930#issuecomment-1754780767
`testTransactionContextIsPassedBack` failed constantly. In my local env,
```java
java.lang.AssertionError
org.apache.ratis.statemachine.TestStateMachine$SMTransactionContext.applyTransaction(TestStateMachine.java:94)
at org.apache.ratis.server.impl.RaftServerImpl.applyLogToStateMachine(RaftServerImpl.java:1842)
at org.apache.ratis.server.impl.StateMachineUpdater.applyLog(StateMachineUpdater.java:244)
at org.apache.ratis.server.impl.StateMachineUpdater.run(StateMachineUpdater.java:186)
at java.lang.Thread.run(Thread.java:750)
```
Guess it's related to `getTransactionContext`. The leader somehow cannot obtain the client request from transaction context.
--
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
Re: [PR] RATIS-1897. Make TransactionContext available in DataApi.write(..). [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #930:
URL: https://github.com/apache/ratis/pull/930#issuecomment-1755956582
@SzyWilliam , you are right that there is a bug in `getTransactionContext`. It should also check leader when `createNew == true`.
--
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