rpuch commented on code in PR #7244:
URL: https://github.com/apache/ignite-3/pull/7244#discussion_r2629782674
##########
modules/core/src/main/java/org/apache/ignite/internal/util/SafeTimeValuesTracker.java:
##########
@@ -30,24 +33,44 @@ public SafeTimeValuesTracker(HybridTimestamp initialValue) {
super(initialValue);
}
- @Override
- public void update(HybridTimestamp newValue, @Nullable Void futureResult) {
+ // Holds successful application context.
+ private long commandIndex;
+ private long commandTerm;
+ private String cmdCls;
+
+ /**
+ * Update safe timestamp.
+ *
+ * @param safeTs The value.
+ * @param commandIndex Command index.
+ * @param commandTerm Command term.
+ * @param cmdCls Command class name.
+ */
+ public void update(HybridTimestamp safeTs, long commandIndex, long
commandTerm, String cmdCls) {
if (!enterBusy()) {
throw new TrackerClosedException();
}
try {
Map.Entry<HybridTimestamp, @Nullable Void> current = this.current;
- IgniteBiTuple<HybridTimestamp, @Nullable Void> newEntry = new
IgniteBiTuple<>(newValue, futureResult);
+ IgniteBiTuple<HybridTimestamp, @Nullable Void> newEntry = new
IgniteBiTuple<>(safeTs, null);
// Entries from the same batch receive equal safe timestamps.
if (comparator.compare(newEntry, current) < 0) {
- throw new AssertionError("Reordering detected: [old=" +
current.getKey() + ", new=" + newEntry.get1() + ']');
+ throw new IgniteInternalException(INTERNAL_ERR,
+ "Reordering detected: [old=" + current.getKey() + ",
new=" + newEntry.get1()
+ + ", index=" + this.commandIndex
+ + ", term=" + this.commandTerm
Review Comment:
Would it make sense to also output new index+term? If not, should we
explicitly mark index+term we output as 'old'?
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java:
##########
@@ -868,13 +871,17 @@ public void onApply(Iterator iter) {
st = new Status(RaftError.ESTATEMACHINE, "Unknown state
machine error.");
}
- // This is necessary so that IndexOutOfBoundsException is not
thrown in a situation where the listener, when processing a
- // command, catch any exception and does clo.result(throwable)
(that actually advances the iterator) and then throws the
- // caught exception.
- Closure done = writeCommandIterator.doneForExceptionHandling();
+ if (iterWrapper.done != null) {
Review Comment:
So we will not log the exception is there is no `done`?
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java:
##########
@@ -1735,6 +1719,24 @@ private void executeApplyingTasks(final
List<LogEntryAndClosure> tasks) {
task.reset();
continue;
}
+
+ // To prevent safe timestamp values from becoming stale, we
must assign them under a valid leader lock.
+ if (task.done instanceof SafeTimeAwareCommandClosure) {
+ SafeTimeAwareCommandClosure clo =
(SafeTimeAwareCommandClosure) task.done;
+ WriteCommand command = clo.command();
+ HybridTimestamp timestamp = command.initiatorTime();
+
+ if (timestamp != null) {
+ if (safeTs == null) {
+ safeTs = clock.update(timestamp);
+ } else if (timestamp.compareTo(safeTs) > 0) {
+ safeTs = clock.update(timestamp);
+ }
+
+ clo.safeTimestamp(safeTs);
+ }
+ }
Review Comment:
Would it make sense to extract this to a method of its own to minimize
intrusion in the original JRaft code?
##########
modules/core/src/main/java/org/apache/ignite/internal/util/SafeTimeValuesTracker.java:
##########
@@ -30,24 +33,44 @@ public SafeTimeValuesTracker(HybridTimestamp initialValue) {
super(initialValue);
}
- @Override
- public void update(HybridTimestamp newValue, @Nullable Void futureResult) {
+ // Holds successful application context.
+ private long commandIndex;
+ private long commandTerm;
+ private String cmdCls;
Review Comment:
```suggestion
private String commandClassName;
```
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/ZonePartitionRaftListener.java:
##########
@@ -161,12 +161,6 @@ public void onWrite(Iterator<CommandClosure<WriteCommand>>
iterator) {
try {
processWriteCommand(clo);
} catch (Throwable t) {
- LOG.error(
Review Comment:
If `clo.result(t)` throws an exception itself, is it guaranteed that it will
log `t` itself? It seems more reliable to still have this `LOG.error()` in the
beginning: if we duplicate logging, it creates a little noise in the log, but
if we miss it completely, we'll not know about the exception at all
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
