Copilot commented on code in PR #7770:
URL: https://github.com/apache/ignite-3/pull/7770#discussion_r2925896574
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/handlers/MinimumActiveTxTimeCommandHandler.java:
##########
@@ -77,6 +77,14 @@ protected CommandResult handleInternally(
long timestamp = command.timestamp();
+ // We MUST bump information about last updated index+term.
+ // See a comment in TablePartitionProcessor#processCommand() for
explanation.
+ storage.runConsistently(locker -> {
+ storage.lastApplied(commandIndex, commandTerm);
Review Comment:
This unconditionally overwrites `lastApplied(index, term)` and can move
`lastAppliedIndex` backwards if a stale command is processed (e.g., after
snapshot install / reordering / retries). To keep `lastApplied` monotonic,
update it only when `commandIndex` is greater than the current
`storage.lastAppliedIndex()` (perform the check inside the same
`runConsistently` closure).
```suggestion
if (commandIndex > storage.lastAppliedIndex()) {
storage.lastApplied(commandIndex, commandTerm);
}
```
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/TablePartitionProcessor.java:
##########
@@ -200,6 +200,14 @@ public CommandResult processCommand(
throw new AssertionError("Unknown command type [command=" +
command.toStringForLightLogging() + ']');
}
+ assert storage.lastAppliedIndex() >= commandIndex : String.format(
+ "Last applied index after command application is less than the
command index "
+ + "[lastAppliedIndex=%d, commandIndex=%d, command=%s]",
+ storage.lastAppliedIndex(),
+ commandIndex,
+ command.toStringForLightLogging()
+ );
Review Comment:
The assertion reads `storage.lastAppliedIndex()` twice. Storing it in a
local variable (e.g., `long lastApplied = storage.lastAppliedIndex();`) makes
the assertion easier to read and avoids any chance of the value changing
between evaluations (or paying the cost twice if the getter is non-trivial).
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/ZonePartitionRaftListener.java:
##########
@@ -539,4 +566,14 @@ private CommandResult handleSafeTimeSyncCommand(long
commandIndex, long commandT
public HybridTimestamp currentSafeTime() {
return safeTimeTracker.current();
}
+
+ private static class CrossTableCommandResult {
+ private final boolean hadAnyTableProcessor;
+ private final CommandResult result;
+
+ private CrossTableCommandResult(boolean hadAnyTableProcessor,
CommandResult result) {
+ this.hadAnyTableProcessor = hadAnyTableProcessor;
+ this.result = result;
+ }
Review Comment:
This is a simple immutable data carrier. If the module is on Java 16+
(likely in Ignite), consider making this a `record` to reduce boilerplate and
make intent clearer (e.g., `private static record
CrossTableCommandResult(boolean hadAnyTableProcessor, CommandResult result)
{}`).
```suggestion
private static record CrossTableCommandResult(boolean
hadAnyTableProcessor, CommandResult result) {
```
--
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]
