Phillippko commented on code in PR #7748:
URL: https://github.com/apache/ignite-3/pull/7748#discussion_r2922971857
##########
modules/raft/src/test/java/org/apache/ignite/internal/raft/storage/segstore/GroupIndexMetaTest.java:
##########
@@ -317,17 +317,98 @@ void testOnIndexCompactedWithMultipleBlocks() {
var compactedMeta1 = new IndexFileMeta(1, 100, 0, new
FileProperties(0, 1));
groupMeta.onIndexCompacted(new FileProperties(0), compactedMeta1);
- assertThat(groupMeta.indexMeta(1), is(compactedMeta1));
- assertThat(groupMeta.indexMeta(42), is(meta2));
- assertThat(groupMeta.indexMeta(100), is(meta3));
+ assertThat(groupMeta.indexMetaByLogIndex(1), is(compactedMeta1));
+ assertThat(groupMeta.indexMetaByLogIndex(42), is(meta2));
+ assertThat(groupMeta.indexMetaByLogIndex(100), is(meta3));
// Compact meta3 from the newer block.
var compactedMeta3 = new IndexFileMeta(100, 200, 84, new
FileProperties(2, 1));
groupMeta.onIndexCompacted(new FileProperties(2), compactedMeta3);
- assertThat(groupMeta.indexMeta(1), is(compactedMeta1));
- assertThat(groupMeta.indexMeta(42), is(meta2));
- assertThat(groupMeta.indexMeta(100), is(compactedMeta3));
+ assertThat(groupMeta.indexMetaByLogIndex(1), is(compactedMeta1));
+ assertThat(groupMeta.indexMetaByLogIndex(42), is(meta2));
+ assertThat(groupMeta.indexMetaByLogIndex(100), is(compactedMeta3));
+ }
+
+ @Test
+ void testIndexMetaByFileOrdinal() {
+ var meta1 = new IndexFileMeta(1, 50, 0, new FileProperties(1));
+ var meta2 = new IndexFileMeta(50, 100, 42, new FileProperties(2));
+ var meta3 = new IndexFileMeta(100, 150, 84, new FileProperties(3));
+
+ var groupMeta = new GroupIndexMeta(meta1);
+ groupMeta.addIndexMeta(meta2);
+ groupMeta.addIndexMeta(meta3);
+
+ assertThat(groupMeta.indexMetaByFileOrdinal(1), is(meta1));
+ assertThat(groupMeta.indexMetaByFileOrdinal(2), is(meta2));
+ assertThat(groupMeta.indexMetaByFileOrdinal(3), is(meta3));
+
+ // Ordinals before the first and after the last return null.
+ assertThat(groupMeta.indexMetaByFileOrdinal(0), is(nullValue()));
+ assertThat(groupMeta.indexMetaByFileOrdinal(4), is(nullValue()));
+ }
+
+ @Test
+ void testIndexMetaByFileOrdinalWithMultipleBlocks() {
+ // meta1 is in block 0.
+ var meta1 = new IndexFileMeta(1, 100, 0, new FileProperties(0));
+ var groupMeta = new GroupIndexMeta(meta1);
+
+ // meta2 overlaps meta1, creating a second deque block.
+ var meta2 = new IndexFileMeta(42, 100, 42, new FileProperties(1));
+ groupMeta.addIndexMeta(meta2);
+
+ // meta3 is appended to the second block (consecutive to meta2).
+ var meta3 = new IndexFileMeta(100, 150, 84, new FileProperties(2));
+ groupMeta.addIndexMeta(meta3);
+
+ assertThat(groupMeta.indexMetaByFileOrdinal(0), is(meta1));
+ assertThat(groupMeta.indexMetaByFileOrdinal(1), is(meta2));
+ assertThat(groupMeta.indexMetaByFileOrdinal(2), is(meta3));
+
+ // Ordinal after the last returns null.
+ assertThat(groupMeta.indexMetaByFileOrdinal(3), is(nullValue()));
Review Comment:
could we check that ordinal before first also returns null, as in previous
test?
##########
modules/raft/src/test/java/org/apache/ignite/internal/raft/storage/segstore/GroupIndexMetaTest.java:
##########
@@ -317,17 +317,98 @@ void testOnIndexCompactedWithMultipleBlocks() {
var compactedMeta1 = new IndexFileMeta(1, 100, 0, new
FileProperties(0, 1));
groupMeta.onIndexCompacted(new FileProperties(0), compactedMeta1);
- assertThat(groupMeta.indexMeta(1), is(compactedMeta1));
- assertThat(groupMeta.indexMeta(42), is(meta2));
- assertThat(groupMeta.indexMeta(100), is(meta3));
+ assertThat(groupMeta.indexMetaByLogIndex(1), is(compactedMeta1));
+ assertThat(groupMeta.indexMetaByLogIndex(42), is(meta2));
+ assertThat(groupMeta.indexMetaByLogIndex(100), is(meta3));
// Compact meta3 from the newer block.
var compactedMeta3 = new IndexFileMeta(100, 200, 84, new
FileProperties(2, 1));
groupMeta.onIndexCompacted(new FileProperties(2), compactedMeta3);
- assertThat(groupMeta.indexMeta(1), is(compactedMeta1));
- assertThat(groupMeta.indexMeta(42), is(meta2));
- assertThat(groupMeta.indexMeta(100), is(compactedMeta3));
+ assertThat(groupMeta.indexMetaByLogIndex(1), is(compactedMeta1));
+ assertThat(groupMeta.indexMetaByLogIndex(42), is(meta2));
+ assertThat(groupMeta.indexMetaByLogIndex(100), is(compactedMeta3));
+ }
+
+ @Test
+ void testIndexMetaByFileOrdinal() {
+ var meta1 = new IndexFileMeta(1, 50, 0, new FileProperties(1));
+ var meta2 = new IndexFileMeta(50, 100, 42, new FileProperties(2));
+ var meta3 = new IndexFileMeta(100, 150, 84, new FileProperties(3));
+
+ var groupMeta = new GroupIndexMeta(meta1);
+ groupMeta.addIndexMeta(meta2);
+ groupMeta.addIndexMeta(meta3);
+
+ assertThat(groupMeta.indexMetaByFileOrdinal(1), is(meta1));
+ assertThat(groupMeta.indexMetaByFileOrdinal(2), is(meta2));
+ assertThat(groupMeta.indexMetaByFileOrdinal(3), is(meta3));
+
+ // Ordinals before the first and after the last return null.
+ assertThat(groupMeta.indexMetaByFileOrdinal(0), is(nullValue()));
+ assertThat(groupMeta.indexMetaByFileOrdinal(4), is(nullValue()));
+ }
+
+ @Test
+ void testIndexMetaByFileOrdinalWithMultipleBlocks() {
+ // meta1 is in block 0.
+ var meta1 = new IndexFileMeta(1, 100, 0, new FileProperties(0));
+ var groupMeta = new GroupIndexMeta(meta1);
+
+ // meta2 overlaps meta1, creating a second deque block.
+ var meta2 = new IndexFileMeta(42, 100, 42, new FileProperties(1));
+ groupMeta.addIndexMeta(meta2);
+
+ // meta3 is appended to the second block (consecutive to meta2).
+ var meta3 = new IndexFileMeta(100, 150, 84, new FileProperties(2));
+ groupMeta.addIndexMeta(meta3);
+
+ assertThat(groupMeta.indexMetaByFileOrdinal(0), is(meta1));
+ assertThat(groupMeta.indexMetaByFileOrdinal(1), is(meta2));
+ assertThat(groupMeta.indexMetaByFileOrdinal(2), is(meta3));
+
+ // Ordinal after the last returns null.
+ assertThat(groupMeta.indexMetaByFileOrdinal(3), is(nullValue()));
+ }
+
+ @Test
+ void testIndexMetaByFileOrdinalAfterTruncatePrefix() {
Review Comment:
Should we test case for files with overlapping metas, with multiple blocks?
##########
modules/raft/src/test/java/org/apache/ignite/internal/raft/storage/segstore/RaftLogGarbageCollectorTest.java:
##########
@@ -155,35 +148,36 @@ void testCompactSegmentFileWithAllEntriesTruncated()
throws Exception {
}
@Test
- void testCompactSegmentFileWithSomeEntriesTruncated() throws Exception {
+ void testRunCompactionWithSomeEntriesTruncated() throws Exception {
Review Comment:
```suggestion
void testRunCompactionWithFileNotTruncatedCompletely() throws Exception {
```
Same, new name is confusing
File is not compacted completely just because there is a second group in it
- why also change size of batches? Let's reduce differences between test cases
to avoid confusion
##########
modules/raft/src/test/java/org/apache/ignite/internal/raft/storage/segstore/RaftLogGarbageCollectorTest.java:
##########
@@ -119,16 +111,17 @@ void tearDown() throws Exception {
}
@Test
- void testCompactSegmentFileWithAllEntriesTruncated() throws Exception {
+ void testRunCompactionWithAllEntriesTruncated() throws Exception {
Review Comment:
```suggestion
void testRunCompactionWithFullyTruncatedFile() throws Exception {
```
(or something else)
I was confused by the test name, didn't understand why we truncate only part
of the entries if we need to truncate all of them
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/IndexFileMetaArray.java:
##########
@@ -208,6 +203,25 @@ IndexFileMetaArray onIndexCompacted(FileProperties
oldProperties, IndexFileMeta
return new IndexFileMetaArray(newArray, size);
}
+ @Nullable
+ IndexFileMeta findByFileOrdinal(int fileOrdinal) {
+ int arrayIndex = arrayIndexByFileOrdinal(fileOrdinal);
+
+ return arrayIndex == -1 ? null : array[arrayIndex];
+ }
+
+ private int arrayIndexByFileOrdinal(int fileOrdinal) {
+ int smallestOrdinal = array[0].indexFileProperties().ordinal();
+
+ if (fileOrdinal < smallestOrdinal) {
+ return -1;
+ }
+
+ int arrayIndex = fileOrdinal - smallestOrdinal;
+
+ return arrayIndex >= size ? -1 : arrayIndex;
Review Comment:
let's replace -1 with a constant
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/RaftLogGarbageCollector.java:
##########
@@ -142,23 +137,61 @@ void cleanupLeftoverFiles() throws IOException {
}
}
- // TODO: Optimize compaction of completely truncated files, see
https://issues.apache.org/jira/browse/IGNITE-27964.
@VisibleForTesting
- void compactSegmentFile(SegmentFile segmentFile) throws IOException {
+ void runCompaction(SegmentFile segmentFile) throws IOException {
LOG.info("Compacting segment file [path = {}].", segmentFile.path());
- // Cache for avoiding excessive min/max log index computations.
- var logStorageInfos = new Long2ObjectOpenHashMap<GroupInfo>();
+ Long2ObjectMap<GroupDescriptor> segmentFileDescription
+ =
indexFileManager.describeSegmentFile(segmentFile.fileProperties().ordinal());
+
+ boolean canRemoveSegmentFile = segmentFileDescription.isEmpty();
+
+ Path indexFilePath =
indexFileManager.indexFilePath(segmentFile.fileProperties());
+
+ long logSizeDelta;
+
+ if (canRemoveSegmentFile) {
+ logSizeDelta = Files.size(segmentFile.path()) +
Files.size(indexFilePath);
+ } else {
+ logSizeDelta = compactSegmentFile(segmentFile, indexFilePath,
segmentFileDescription);
+ }
+ // Remove the previous generation of the segment file and its index.
This is safe to do, because we rely on the file system
+ // guarantees that other threads reading from the segment file will
still be able to do that even if the file is deleted.
+ Files.delete(segmentFile.path());
+ Files.delete(indexFilePath);
+
+ long newLogSize = logSize.addAndGet(-logSizeDelta);
+
+ if (LOG.isInfoEnabled()) {
+ if (canRemoveSegmentFile) {
+ LOG.info(
+ "Segment file removed (all entries are truncated)
[path = {}, log size freed = {}, new log size = {}].",
Review Comment:
```suggestion
"Segment file removed (all entries are truncated)
[path = {}, log size freed = {} bytes, new log size = {} bytes].",
```
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/RaftLogGarbageCollector.java:
##########
@@ -142,23 +137,61 @@ void cleanupLeftoverFiles() throws IOException {
}
}
- // TODO: Optimize compaction of completely truncated files, see
https://issues.apache.org/jira/browse/IGNITE-27964.
@VisibleForTesting
- void compactSegmentFile(SegmentFile segmentFile) throws IOException {
+ void runCompaction(SegmentFile segmentFile) throws IOException {
LOG.info("Compacting segment file [path = {}].", segmentFile.path());
- // Cache for avoiding excessive min/max log index computations.
- var logStorageInfos = new Long2ObjectOpenHashMap<GroupInfo>();
+ Long2ObjectMap<GroupDescriptor> segmentFileDescription
+ =
indexFileManager.describeSegmentFile(segmentFile.fileProperties().ordinal());
+
+ boolean canRemoveSegmentFile = segmentFileDescription.isEmpty();
+
+ Path indexFilePath =
indexFileManager.indexFilePath(segmentFile.fileProperties());
+
+ long logSizeDelta;
+
+ if (canRemoveSegmentFile) {
+ logSizeDelta = Files.size(segmentFile.path()) +
Files.size(indexFilePath);
+ } else {
+ logSizeDelta = compactSegmentFile(segmentFile, indexFilePath,
segmentFileDescription);
+ }
+ // Remove the previous generation of the segment file and its index.
This is safe to do, because we rely on the file system
+ // guarantees that other threads reading from the segment file will
still be able to do that even if the file is deleted.
+ Files.delete(segmentFile.path());
+ Files.delete(indexFilePath);
+
+ long newLogSize = logSize.addAndGet(-logSizeDelta);
Review Comment:
```suggestion
long newLogSize = logSizeBytes.addAndGet(-logSizeDelta);
```
or something else, now it's not obvious that size in bytes and not in log
records or something
##########
modules/raft/src/test/java/org/apache/ignite/internal/raft/storage/segstore/RaftLogGarbageCollectorTest.java:
##########
@@ -199,66 +193,60 @@ void testCompactSegmentFileWithSomeEntriesTruncated()
throws Exception {
}
@Test
- void testCompactSegmentFileWithTruncationRecords() throws Exception {
+ void testRunCompactionWithTruncationRecords() throws Exception {
List<byte[]> batches = createRandomData(FILE_SIZE / 4, 5);
for (int i = 0; i < batches.size(); i++) {
appendBytes(GROUP_ID_1, batches.get(i), i);
}
- fileManager.truncatePrefix(GROUP_ID_1, 2);
- fileManager.truncateSuffix(GROUP_ID_1, 3);
-
- await().until(this::indexFiles, hasSize(greaterThan(0)));
+ // Truncate both prefix and suffix.
+ fileManager.truncatePrefix(GROUP_ID_1, batches.size() / 2);
+ fileManager.truncateSuffix(GROUP_ID_1, batches.size() / 2);
List<Path> segmentFiles = segmentFiles();
- assertThat(segmentFiles, hasSize(greaterThan(1)));
- // This is equivalent to prefix truncation up to the latest index.
- when(groupInfoProvider.groupInfo(GROUP_ID_1)).thenReturn(new
GroupInfo(batches.size() - 1, batches.size() - 1));
+ triggerAndAwaitCheckpoint(batches.size() / 2);
- Path firstSegmentFile = segmentFiles.get(0);
+ for (Path segmentFilePath : segmentFiles) {
+ SegmentFile segmentFile =
SegmentFile.openExisting(segmentFilePath, false);
- SegmentFile segmentFile = SegmentFile.openExisting(firstSegmentFile,
false);
- try {
- garbageCollector.compactSegmentFile(segmentFile);
- } finally {
- segmentFile.close();
- }
+ try {
+ garbageCollector.runCompaction(segmentFile);
+ } finally {
+ segmentFile.close();
+ }
- assertThat(Files.exists(firstSegmentFile), is(false));
+ assertThat(Files.exists(segmentFilePath), is(false));
Review Comment:
```suggestion
assertThat(segmentFilePath, not(exists()));
```
--
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]
