sashapolo commented on code in PR #5684:
URL: https://github.com/apache/ignite-3/pull/5684#discussion_r2058492309
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/KeyValueStorageUtils.java:
##########
@@ -43,14 +43,20 @@ public static int indexToCompact(long[] keyRevisions, long
compactionRevisionInc
int i = binarySearch(keyRevisions, compactionRevisionInclusive);
if (i < 0) {
- if (i == -1) {
+ i = ~i;
+
+ if (i == 0) {
return NOT_FOUND;
}
- i = -(i + 2);
+ i--;
}
- if (i == keyRevisions.length - 1 &&
!isTombstone.test(keyRevisions[i])) {
+ if (!isTombstone.test(keyRevisions[i])) {
+ if (i != keyRevisions.length - 1 && keyRevisions[i + 1] ==
compactionRevisionInclusive + 1) {
Review Comment:
Please add some comments about this condition
##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/server/AbstractCompactionKeyValueStorageTest.java:
##########
@@ -510,6 +510,18 @@ void testGetSingleEntryAndCompactionForFooKey() {
assertDoesNotThrowCompactedExceptionForGetSingleValue(FOO_KEY, 5);
}
+ @Test
+ void testDoNotCompactOnExactMatch() {
Review Comment:
So we actually do not want to compact on exact match? Should we update the
javadoc?
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/KeyValueStorageUtils.java:
##########
@@ -43,14 +43,20 @@ public static int indexToCompact(long[] keyRevisions, long
compactionRevisionInc
int i = binarySearch(keyRevisions, compactionRevisionInclusive);
if (i < 0) {
- if (i == -1) {
+ i = ~i;
+
+ if (i == 0) {
return NOT_FOUND;
}
- i = -(i + 2);
+ i--;
}
- if (i == keyRevisions.length - 1 &&
!isTombstone.test(keyRevisions[i])) {
+ if (!isTombstone.test(keyRevisions[i])) {
Review Comment:
I would propose to invert this condition, this will make the code more
readable
##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/server/AbstractCompactionKeyValueStorageTest.java:
##########
@@ -157,7 +157,7 @@ public void tearDown() throws Exception {
void testCompactRevision1() {
storage.compact(1);
- assertEquals(List.of(3, 5), collectRevisions(FOO_KEY));
+ assertEquals(List.of(1, 3, 5), collectRevisions(FOO_KEY));
Review Comment:
`compact` method's javadoc says:
```
@param revision Revision up to which (including) the metastorage keys will
be compacted
```
Why do we have the `1` revision here? And why doesn't `BAR_KEY` have this
revision?
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/KeyValueStorageUtils.java:
##########
@@ -43,14 +43,20 @@ public static int indexToCompact(long[] keyRevisions, long
compactionRevisionInc
int i = binarySearch(keyRevisions, compactionRevisionInclusive);
if (i < 0) {
- if (i == -1) {
+ i = ~i;
+
+ if (i == 0) {
return NOT_FOUND;
}
- i = -(i + 2);
+ i--;
}
- if (i == keyRevisions.length - 1 &&
!isTombstone.test(keyRevisions[i])) {
+ if (!isTombstone.test(keyRevisions[i])) {
Review Comment:
Also, if `keyRevisions[i]` is a tombstone, but the
`compactionRevisionInclusive` was not found in the keyRevisions array, then we
would return the index of the first element that is greater
`compactionRevisionInclusive`. Doesn't it look strange 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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
