Copilot commented on code in PR #7958:
URL: https://github.com/apache/ignite-3/pull/7958#discussion_r3052690733
##########
modules/raft/src/test/java/org/apache/ignite/internal/raft/storage/segstore/RaftLogGarbageCollectorTest.java:
##########
@@ -646,6 +646,90 @@ void
testCompactionOfFileAdjacentToStaleEntryInDequeCausesCorruption() throws Ex
});
}
+ /**
+ * Verifies that after {@link SegmentFileManager#destroyGroup} is called,
the GC can remove segment files belonging
+ * to the destroyed group.
+ */
+ @Test
+ void testSegmentFilesRemovedByGcAfterGroupDestroy() throws Exception {
+ List<byte[]> batches = createRandomData(FILE_SIZE / 4, 10);
+
+ for (int i = 0; i < batches.size(); i++) {
+ appendBytes(GROUP_ID_1, batches.get(i), i);
+ }
+
+ await().until(this::indexFiles, hasSize(equalTo(segmentFiles().size()
- 1)));
+
+ List<Path> oldSegmentFiles = segmentFiles().subList(0,
segmentFiles().size() - 1);
+ List<Path> oldIndexFiles = indexFiles();
+
+ fileManager.destroyGroup(GROUP_ID_1);
+
+ // The destroy tombstone is in the current segment file's memtable.
Trigger a rollover using a different group so the
+ // checkpoint thread processes the tombstone and removes GROUP_ID_1
from the in-memory index.
+ triggerAndAwaitCheckpoint(GROUP_ID_2, 0);
+
Review Comment:
These tests verify that segment/index files are deleted/compacted after
`destroyGroup`, but they don’t verify the primary correctness property: entries
from the destroyed group must become unreadable via `getEntry`. Because the
test data uses log index 0, a destroy implemented as `reset(..., 0)` can still
leave index 0 readable (depending on existing memtable state), which would not
be caught here. Please add assertions that `getEntry(GROUP_ID_1, i)` returns
null for the previously appended indices (including 0), and/or consider using
1-based indices in the test to match JRaft’s empty-log contract
(first=1,last=0).
```suggestion
for (int i = 0; i < batches.size(); i++) {
assertThat(getEntry(GROUP_ID_1, i), is((LogEntry) null));
}
```
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/IndexFileManager.java:
##########
@@ -451,6 +451,12 @@ private void putIndexFileMeta(IndexMetaSpec metaSpec) {
long firstIndexKept = metaSpec.firstIndexKept();
+ if (firstIndexKept == GROUP_DESTROY_LOG_INDEX) {
+ groupIndexMetas.remove(groupId);
+
+ return;
+ }
Review Comment:
This removal is keyed off `firstIndexKept == GROUP_DESTROY_LOG_INDEX`, but
`firstIndexKept` is also used for regular prefix/reset tombstones and can
legitimately be `0` if a group ever uses log index 0 (the segstore tests do).
In that case, this would incorrectly drop all in-memory index metadata for an
active group, making entries unreachable and potentially causing the GC to
delete/compact files incorrectly. Please make the “group destroyed” signal
unambiguous (e.g., a separate persisted flag/record marker) or add a strict
invariant (validated at write/reset boundaries) that log indices are always >=
1 so `0` can never occur except as a destroy marker.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/SegmentFileManager.java:
##########
@@ -114,6 +114,11 @@ class SegmentFileManager implements ManuallyCloseable {
*/
static final byte[] SWITCH_SEGMENT_RECORD = new byte[8]; // 8 zero bytes.
+ /**
+ * Special "destroy group" sentinel value for the log reset index.
+ */
+ static final long GROUP_DESTROY_LOG_INDEX = 0L;
Review Comment:
`GROUP_DESTROY_LOG_INDEX` is set to `0`, but the segstore code does not
enforce log indices to be > 0 (tests in this module regularly append index 0).
Overloading a valid log index as a destroy sentinel can lead to ambiguous
semantics (e.g., reset may keep the entry at index 0 instead of fully
discarding the group) and can also poison recovery:
`firstLogIndexInclusiveOnRecovery` prefers `segmentInfo.firstIndexKept()`, so
after a destroy tombstone it can return 0, which makes
`SegstoreLogStorage.getFirstLogIndex()` return 0 (JRaft expects 1 for an empty
log). Consider making the destroy marker unambiguous (dedicated record
type/flag) or enforce/validate that all stored log indices are >= 1 and handle
the destroy tombstone explicitly on recovery so destroyed groups recover as
empty (first=1,last=0).
```suggestion
* Must not overlap with a valid stored log index.
*/
static final long GROUP_DESTROY_LOG_INDEX = -1L;
```
--
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]
