You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ascherbakoff (via GitHub)" <gi...@apache.org> on 2025/01/27 11:49:05 UTC
[PR] IGNITE-24283 Remove shared command from replication pipeline. [ignite-3]
ascherbakoff opened a new pull request, #5124:
URL: https://github.com/apache/ignite-3/pull/5124
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935616191
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/ActionRequestProcessor.java:
##########
@@ -52,7 +52,7 @@
import org.apache.ignite.raft.jraft.rpc.RpcProcessor;
import org.apache.ignite.raft.jraft.rpc.RpcRequests;
import org.apache.ignite.raft.jraft.rpc.WriteActionRequest;
-import org.apache.ignite.raft.jraft.util.BytesUtil;
+import org.apache.ignite.raft.jraft.util.BytesUtil;import org.jetbrains.annotations.Nullable;
Review Comment:
🆗
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935375638
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/PartitionListener.java:
##########
@@ -270,12 +281,7 @@ private IgniteBiTuple<Serializable, Boolean> handleUpdateCommand(UpdateCommand c
long storageLeaseStartTime = storage.leaseStartTime();
if (leaseStartTime != storageLeaseStartTime) {
- return new IgniteBiTuple<>(new UpdateCommandResult(
- false,
- storageLeaseStartTime,
- isPrimaryInGroupTopology(),
- cmd.safeTime().longValue()
- ), false);
+ return new IgniteBiTuple<>(new UpdateCommandResult(false, storageLeaseStartTime, isPrimaryInGroupTopology(), 0), false);
Review Comment:
This value means 'null'. In case of lease mismatch it's not used.
Replaced with HybridTimestamp.NULL_HYBRID_TIMESTAMP
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935375638
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/PartitionListener.java:
##########
@@ -270,12 +281,7 @@ private IgniteBiTuple<Serializable, Boolean> handleUpdateCommand(UpdateCommand c
long storageLeaseStartTime = storage.leaseStartTime();
if (leaseStartTime != storageLeaseStartTime) {
- return new IgniteBiTuple<>(new UpdateCommandResult(
- false,
- storageLeaseStartTime,
- isPrimaryInGroupTopology(),
- cmd.safeTime().longValue()
- ), false);
+ return new IgniteBiTuple<>(new UpdateCommandResult(false, storageLeaseStartTime, isPrimaryInGroupTopology(), 0), false);
Review Comment:
This value means 'null'. In case of lease mismatch it's not used.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935162023
##########
modules/raft-api/src/main/java/org/apache/ignite/internal/raft/WriteCommand.java:
##########
@@ -34,9 +34,11 @@ public interface WriteCommand extends Command {
}
/**
- * This is called before a command is submitted to replication pipeline.
+ * Get safe timestamp. TODO IGNITE-24143 Move to SafeTimePropagatingCommand.
Review Comment:
🆗
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935377766
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -2906,18 +2906,20 @@ private CompletableFuture<CommandApplicationResult> applyUpdateAllCommand(
return safeTime.waitFor(safeTs)
.thenApply(ignored -> new CommandApplicationResult(safeTs, null));
} else {
+ HybridTimestamp safeTs = ((UpdateAllCommand) res.getCommand()).safeTime();
Review Comment:
Good cache. This is incorrect, fixed.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935374757
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/PartitionListener.java:
##########
@@ -170,6 +170,8 @@ public void onWrite(Iterator<CommandClosure<WriteCommand>> iterator) {
long commandIndex = clo.index();
long commandTerm = clo.term();
+ @Nullable HybridTimestamp safeTimestamp = clo.safeTimestamp();
+ assert safeTimestamp == null || command instanceof SafeTimePropagatingCommand;
Review Comment:
🆗
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935372594
##########
modules/table/src/integrationTest/java/org/apache/ignite/distributed/ReplicasSafeTimePropagationTest.java:
##########
@@ -107,6 +117,11 @@ public class ReplicasSafeTimePropagationTest extends IgniteAbstractTest {
private Map<String, PartialNode> cluster;
+ private volatile boolean blockFsmThread = false;
+
+ private CountDownLatch l1 = new CountDownLatch(1);
+ private CountDownLatch l2 = new CountDownLatch(1);
Review Comment:
🆗
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935165420
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java:
##########
@@ -911,6 +912,7 @@ public CommandClosure<WriteCommand> next() {
ByteBuffer data = iter.getData();
WriteCommand command = done == null ? marshaller.unmarshall(data) : done.command();
+ HybridTimestamp safeTs = done == null ? command.safeTime() : done.safeTimestamp();
Review Comment:
🆗
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935369252
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java:
##########
@@ -314,7 +314,7 @@ public void onEvent(final LogEntryAndClosure event, final long sequence, final b
safeTs = clock.update(timestamp);
Review Comment:
Good question.
Looks like it might break the invariant: safeTs after initiatorTime
Fixed
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java:
##########
@@ -314,7 +314,7 @@ public void onEvent(final LogEntryAndClosure event, final long sequence, final b
safeTs = clock.update(timestamp);
Review Comment:
Good question.
Looks like it might break the invariant: safeTs is after initiatorTime
Fixed
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935383603
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/UpdateCommandResult.java:
##########
@@ -36,6 +36,7 @@ public class UpdateCommandResult implements Serializable {
/** {@code true} if primary replica belongs to the raft group topology: peers and learners, (@code false) otherwise. */
private final boolean primaryInPeersAndLearners;
+ /** The safe timestamp. */
Review Comment:
0 (HybridTimestamp.NULL_HYBRID_TIMESTAMP) means value is 'null'.
See the change above.
Not sure it needs addition comment.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1933857538
##########
modules/raft-api/src/main/java/org/apache/ignite/internal/raft/WriteCommand.java:
##########
@@ -34,9 +34,11 @@ public interface WriteCommand extends Command {
}
/**
- * This is called before a command is submitted to replication pipeline.
+ * Get safe timestamp. TODO IGNITE-24143 Move to SafeTimePropagatingCommand.
Review Comment:
```suggestion
* Gets safe timestamp. TODO IGNITE-24143 Move to SafeTimePropagatingCommand.
```
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/ActionRequestProcessor.java:
##########
@@ -52,7 +52,7 @@
import org.apache.ignite.raft.jraft.rpc.RpcProcessor;
import org.apache.ignite.raft.jraft.rpc.RpcRequests;
import org.apache.ignite.raft.jraft.rpc.WriteActionRequest;
-import org.apache.ignite.raft.jraft.util.BytesUtil;
+import org.apache.ignite.raft.jraft.util.BytesUtil;import org.jetbrains.annotations.Nullable;
Review Comment:
Two imports on one line
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java:
##########
@@ -911,6 +912,7 @@ public CommandClosure<WriteCommand> next() {
ByteBuffer data = iter.getData();
WriteCommand command = done == null ? marshaller.unmarshall(data) : done.command();
+ HybridTimestamp safeTs = done == null ? command.safeTime() : done.safeTimestamp();
Review Comment:
Would it make sense to add a comment that `done != null` means that we are executing a just-submitted command on the leader, and that `done == null` means that the command was deserialized from the log?
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RetryContext.java:
##########
@@ -132,10 +132,7 @@ void resetUnavailablePeers() {
* @return {@code this}.
*/
RetryContext nextAttempt(Peer newTargetPeer) {
- // We can avoid recreating the request if the target peer has not changed.
- if (!newTargetPeer.equals(targetPeer)) {
Review Comment:
Why is this removed? It seems it saves us some request instantiations
##########
modules/table/src/integrationTest/java/org/apache/ignite/distributed/ReplicasSafeTimePropagationTest.java:
##########
@@ -107,6 +117,11 @@ public class ReplicasSafeTimePropagationTest extends IgniteAbstractTest {
private Map<String, PartialNode> cluster;
+ private volatile boolean blockFsmThread = false;
+
+ private CountDownLatch l1 = new CountDownLatch(1);
+ private CountDownLatch l2 = new CountDownLatch(1);
Review Comment:
```suggestion
private final CountDownLatch l1 = new CountDownLatch(1);
private final CountDownLatch l2 = new CountDownLatch(1);
```
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java:
##########
@@ -2906,18 +2906,20 @@ private CompletableFuture<CommandApplicationResult> applyUpdateAllCommand(
return safeTime.waitFor(safeTs)
.thenApply(ignored -> new CommandApplicationResult(safeTs, null));
} else {
+ HybridTimestamp safeTs = ((UpdateAllCommand) res.getCommand()).safeTime();
Review Comment:
Why do we take safeTs from command and not result? And why wasn't this changed for `UpdateCommand` handling where everything seems to be the same?
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java:
##########
@@ -314,7 +314,7 @@ public void onEvent(final LogEntryAndClosure event, final long sequence, final b
safeTs = clock.update(timestamp);
Review Comment:
`clock.update()` is invoked once per batch, and for its first element. What if one of subsequent batch elements has `initiatorTime()` that is more than `safeTs` we generated for first element? Wouldn't it break something?
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/ActionRequestProcessor.java:
##########
@@ -195,9 +202,12 @@ public void run(Status status) {
}
@Override
- public void patch(HybridTimestamp safeTs) {
+ public void safeTime(HybridTimestamp safeTs) {
+ assert this.safeTs == null : "Safe time can be set only once";
Review Comment:
This assert makes sure that `safeTime()` is not set multiple times. But if it actually tries to be invoked more than once, don't we need `safeTs` to be volatile to see that it has already been set earlier? Wouldn't such calls happen from different threads?
##########
modules/raft-api/src/main/java/org/apache/ignite/internal/raft/service/SafeTimeAwareCommandClosure.java:
##########
@@ -17,10 +17,26 @@
package org.apache.ignite.internal.raft.service;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
import org.apache.ignite.internal.raft.WriteCommand;
+import org.jetbrains.annotations.Nullable;
/**
- * The marker interface for a write command closure.
+ * The marker interface for a safe time aware command closure.
*/
-public interface WriteCommandClosure extends CommandClosure<WriteCommand> {
+public interface SafeTimeAwareCommandClosure extends CommandClosure<WriteCommand> {
+ /**
+ * Get the safe timestamp.
+ *
+ * @return The timestamp.
+ */
+ @Override
+ @Nullable HybridTimestamp safeTimestamp();
+
+ /**
+ * Set the safe time.
+ *
+ * @param safeTs Safe timestamp.
+ */
+ default void safeTime(HybridTimestamp safeTs) {}
Review Comment:
Getter and setter names don't agree: one is for `safeTime`, another is for `safeTimestamp`, but they are actually about the same concept
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/UpdateCommandResult.java:
##########
@@ -36,6 +36,7 @@ public class UpdateCommandResult implements Serializable {
/** {@code true} if primary replica belongs to the raft group topology: peers and learners, (@code false) otherwise. */
private final boolean primaryInPeersAndLearners;
+ /** The safe timestamp. */
Review Comment:
According to the code, 0 might be assigned to this field instead of the real safe time requested by the command. If this behavior will be retained, please mention here that 0 can be stored in the field and what it means.
##########
modules/table/src/integrationTest/java/org/apache/ignite/distributed/ReplicasSafeTimePropagationTest.java:
##########
@@ -122,14 +137,19 @@ private static void sendSafeTimeSyncCommand(
RaftGroupService raftClient,
HybridTimestamp initiatorTime
) {
- CompletableFuture<Object> safeTimeCommandFuture = raftClient.run(
+ sendSafeTimeSyncCommandAsync(raftClient, initiatorTime).join();
Review Comment:
I suggest using `assertThat(future, willCompleteSuccessfully());` instead of `join()` as the latter might hang forever, while the former will fail the test explaining what happened
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/PartitionListener.java:
##########
@@ -270,12 +281,7 @@ private IgniteBiTuple<Serializable, Boolean> handleUpdateCommand(UpdateCommand c
long storageLeaseStartTime = storage.leaseStartTime();
if (leaseStartTime != storageLeaseStartTime) {
- return new IgniteBiTuple<>(new UpdateCommandResult(
- false,
- storageLeaseStartTime,
- isPrimaryInGroupTopology(),
- cmd.safeTime().longValue()
- ), false);
+ return new IgniteBiTuple<>(new UpdateCommandResult(false, storageLeaseStartTime, isPrimaryInGroupTopology(), 0), false);
Review Comment:
Why is 0 returned here? Are the callers ready to get it?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/PartitionListener.java:
##########
@@ -170,6 +170,8 @@ public void onWrite(Iterator<CommandClosure<WriteCommand>> iterator) {
long commandIndex = clo.index();
long commandTerm = clo.term();
+ @Nullable HybridTimestamp safeTimestamp = clo.safeTimestamp();
+ assert safeTimestamp == null || command instanceof SafeTimePropagatingCommand;
Review Comment:
```suggestion
assert safeTimestamp == null || command instanceof SafeTimePropagatingCommand : command;
```
##########
modules/table/src/integrationTest/java/org/apache/ignite/distributed/ReplicasSafeTimePropagationTest.java:
##########
@@ -254,6 +274,45 @@ public void testSafeTimeReorderingOnClusterShrink() throws Exception {
assertTrue(leaderNode2.safeTs.current().compareTo(firstSafeTs) > 0);
}
+ @Test
+ public void testSafeTimeReorderingOnRetry() throws Exception {
+ {
+ cluster = Stream.of("node1").collect(toMap(identity(), PartialNode::new));
+
+ startCluster(cluster);
+ }
+
+ PartialNode someNode = cluster.values().iterator().next();
+
+ RaftGroupService raftClient = someNode.raftClient;
+
+ assertThat(raftClient.refreshLeader(), willCompleteSuccessfully());
+
+ // Assumes stable clock on test runner.
+ HybridClock initiatorClock = new TestHybridClock(() -> System.currentTimeMillis() - 100);
+
+ sendSafeTimeSyncCommand(raftClient, initiatorClock.now());
+
+ blockFsmThread = true;
+
+ var fut = sendSafeTimeSyncCommandAsync(raftClient, initiatorClock.now());
+ var fut2 = sendSafeTimeSyncCommandAsync(raftClient, initiatorClock.now());
+
+ l1.await();
+
+ // Block enough to trigger retry.
+ Thread.sleep(raftConfiguration.responseTimeout().value() + 1000);
+
+ LOG.info("Unblocking FSM thread");
+
+ blockFsmThread = false;
+
+ l2.countDown();
+
+ fut.join();
+ fut2.join();
Review Comment:
```suggestion
assertThat(fut, willCompleteSuccessfully());
assertThat(fut2, willCompleteSuccessfully());
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935155220
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RetryContext.java:
##########
@@ -132,10 +132,7 @@ void resetUnavailablePeers() {
* @return {@code this}.
*/
RetryContext nextAttempt(Peer newTargetPeer) {
- // We can avoid recreating the request if the target peer has not changed.
- if (!newTargetPeer.equals(targetPeer)) {
Review Comment:
We need to recreate request each time to make sure the associated binary command is always new.
Otherwise it's state is shared between current request and retry, causing possible races during binary patch.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935383603
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/UpdateCommandResult.java:
##########
@@ -36,6 +36,7 @@ public class UpdateCommandResult implements Serializable {
/** {@code true} if primary replica belongs to the raft group topology: peers and learners, (@code false) otherwise. */
private final boolean primaryInPeersAndLearners;
+ /** The safe timestamp. */
Review Comment:
0 (HybridTimestamp.NULL_HYBRID_TIMESTAMP) means value is 'null'. Not sure it needs addition comment.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935414971
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/UpdateCommandResult.java:
##########
@@ -36,6 +36,7 @@ public class UpdateCommandResult implements Serializable {
/** {@code true} if primary replica belongs to the raft group topology: peers and learners, (@code false) otherwise. */
private final boolean primaryInPeersAndLearners;
+ /** The safe timestamp. */
Review Comment:
It could be useful for someone who is new to the code. Up to you
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935371371
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/ActionRequestProcessor.java:
##########
@@ -195,9 +202,12 @@ public void run(Status status) {
}
@Override
- public void patch(HybridTimestamp safeTs) {
+ public void safeTime(HybridTimestamp safeTs) {
+ assert this.safeTs == null : "Safe time can be set only once";
Review Comment:
This method should only be invoked from raft event loop.
So it's safe without volatile.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935374119
##########
modules/table/src/integrationTest/java/org/apache/ignite/distributed/ReplicasSafeTimePropagationTest.java:
##########
@@ -254,6 +274,45 @@ public void testSafeTimeReorderingOnClusterShrink() throws Exception {
assertTrue(leaderNode2.safeTs.current().compareTo(firstSafeTs) > 0);
}
+ @Test
+ public void testSafeTimeReorderingOnRetry() throws Exception {
+ {
+ cluster = Stream.of("node1").collect(toMap(identity(), PartialNode::new));
+
+ startCluster(cluster);
+ }
+
+ PartialNode someNode = cluster.values().iterator().next();
+
+ RaftGroupService raftClient = someNode.raftClient;
+
+ assertThat(raftClient.refreshLeader(), willCompleteSuccessfully());
+
+ // Assumes stable clock on test runner.
+ HybridClock initiatorClock = new TestHybridClock(() -> System.currentTimeMillis() - 100);
+
+ sendSafeTimeSyncCommand(raftClient, initiatorClock.now());
+
+ blockFsmThread = true;
+
+ var fut = sendSafeTimeSyncCommandAsync(raftClient, initiatorClock.now());
+ var fut2 = sendSafeTimeSyncCommandAsync(raftClient, initiatorClock.now());
+
+ l1.await();
+
+ // Block enough to trigger retry.
+ Thread.sleep(raftConfiguration.responseTimeout().value() + 1000);
+
+ LOG.info("Unblocking FSM thread");
+
+ blockFsmThread = false;
+
+ l2.countDown();
+
+ fut.join();
+ fut2.join();
Review Comment:
🆗
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935373607
##########
modules/table/src/integrationTest/java/org/apache/ignite/distributed/ReplicasSafeTimePropagationTest.java:
##########
@@ -122,14 +137,19 @@ private static void sendSafeTimeSyncCommand(
RaftGroupService raftClient,
HybridTimestamp initiatorTime
) {
- CompletableFuture<Object> safeTimeCommandFuture = raftClient.run(
+ sendSafeTimeSyncCommandAsync(raftClient, initiatorTime).join();
Review Comment:
🆗
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix data race in raft pipeline on command retry [ignite-3]
Posted by "ptupitsyn (via GitHub)" <gi...@apache.org>.
ptupitsyn merged PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935162794
##########
modules/raft-api/src/main/java/org/apache/ignite/internal/raft/service/SafeTimeAwareCommandClosure.java:
##########
@@ -17,10 +17,26 @@
package org.apache.ignite.internal.raft.service;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
import org.apache.ignite.internal.raft.WriteCommand;
+import org.jetbrains.annotations.Nullable;
/**
- * The marker interface for a write command closure.
+ * The marker interface for a safe time aware command closure.
*/
-public interface WriteCommandClosure extends CommandClosure<WriteCommand> {
+public interface SafeTimeAwareCommandClosure extends CommandClosure<WriteCommand> {
+ /**
+ * Get the safe timestamp.
+ *
+ * @return The timestamp.
+ */
+ @Override
+ @Nullable HybridTimestamp safeTimestamp();
+
+ /**
+ * Set the safe time.
+ *
+ * @param safeTs Safe timestamp.
+ */
+ default void safeTime(HybridTimestamp safeTs) {}
Review Comment:
🆗
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24283 Fix a race in safe timestamp processing during raft command retry. [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #5124:
URL: https://github.com/apache/ignite-3/pull/5124#discussion_r1935383603
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/UpdateCommandResult.java:
##########
@@ -36,6 +36,7 @@ public class UpdateCommandResult implements Serializable {
/** {@code true} if primary replica belongs to the raft group topology: peers and learners, (@code false) otherwise. */
private final boolean primaryInPeersAndLearners;
+ /** The safe timestamp. */
Review Comment:
0 (HybridTimestamp.NULL_HYBRID_TIMESTAMP) means value is 'null'.
See the change above.
Not sure it needs additional comment.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org