This is an automated email from the ASF dual-hosted git repository.
Jackie-Jiang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 9c02213b99e Do not override user-configured validDocIdsType for upsert
compaction (#18850)
9c02213b99e is described below
commit 9c02213b99e1b6fd88d60401a889acc57b7f5122
Author: Krishan Goyal <[email protected]>
AuthorDate: Tue Jun 30 03:53:04 2026 +0530
Do not override user-configured validDocIdsType for upsert compaction
(#18850)
---
.../pinot/plugin/minion/tasks/MinionTaskUtils.java | 27 +++++++---------------
.../plugin/minion/tasks/MinionTaskUtilsTest.java | 26 +++++++++++----------
2 files changed, 22 insertions(+), 31 deletions(-)
diff --git
a/pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtils.java
b/pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtils.java
index b96b9a56990..ea2150644c3 100644
---
a/pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtils.java
+++
b/pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtils.java
@@ -441,11 +441,12 @@ public class MinionTaskUtils {
/**
* Get the validDocIdsType based on the upsertConfig and taskConfigs.
- * The default value is determined by whether delete is enabled in the
upsertConfig. If delete is enabled,
- * the default value is 'snapshot_with_delete', otherwise it is 'snapshot'.
- * If delete is enabled, we override the user-specified value to
'snapshot_with_delete' for backward compatibility
- * except when it is 'in_memory_with_delete'.
- * It also validates the combination of validDocIdsType, snapshot and
deleteRecordColumn.
+ * The default value is 'snapshot' It validates the combination of
validDocIdsType, snapshot and
+ * deleteRecordColumn:
+ * <ul>
+ * <li>'snapshot' and 'snapshot_with_delete' require upsert snapshots to
be enabled.</li>
+ * <li>'snapshot_with_delete' and 'in_memory_with_delete' require a
deleteRecordColumn to be configured.</li>
+ * </ul>
* @param upsertConfig upsertConfig of the table
* @param taskConfigs taskConfigs of the task
* @param validDocIdsTypeKey the key to get validDocIdsType from taskConfigs
@@ -453,22 +454,10 @@ public class MinionTaskUtils {
*/
public static ValidDocIdsType getValidDocIdsType(UpsertConfig upsertConfig,
Map<String, String> taskConfigs,
String validDocIdsTypeKey) {
- boolean isDeleteEnabled =
StringUtils.isNotEmpty(upsertConfig.getDeleteRecordColumn());
- ValidDocIdsType defaultValidDocIdsType =
- isDeleteEnabled ? ValidDocIdsType.SNAPSHOT_WITH_DELETE :
ValidDocIdsType.SNAPSHOT;
String validDocIdsTypeStr = taskConfigs.getOrDefault(validDocIdsTypeKey,
- defaultValidDocIdsType.name()).toUpperCase();
+ ValidDocIdsType.SNAPSHOT.name()).toUpperCase();
ValidDocIdsType validDocIdsType =
ValidDocIdsType.valueOf(validDocIdsTypeStr);
- if (isDeleteEnabled && validDocIdsType !=
ValidDocIdsType.SNAPSHOT_WITH_DELETE
- && validDocIdsType != ValidDocIdsType.IN_MEMORY_WITH_DELETE) {
- LOGGER.warn(
- "Overriding user-specified validDocIdsType '{}' to '{}' for backward
compatibility because delete is "
- + "enabled (deleteRecordColumn='{}').",
- validDocIdsType, ValidDocIdsType.SNAPSHOT_WITH_DELETE,
upsertConfig.getDeleteRecordColumn());
- validDocIdsType = ValidDocIdsType.SNAPSHOT_WITH_DELETE;
- }
-
if (validDocIdsType == ValidDocIdsType.SNAPSHOT || validDocIdsType ==
ValidDocIdsType.SNAPSHOT_WITH_DELETE) {
Preconditions.checkState(upsertConfig.getSnapshot() !=
Enablement.DISABLE,
"'snapshot' must not be 'DISABLE' with validDocIdsType: %s",
validDocIdsType);
@@ -476,7 +465,7 @@ public class MinionTaskUtils {
if (validDocIdsType == ValidDocIdsType.IN_MEMORY_WITH_DELETE
|| validDocIdsType == ValidDocIdsType.SNAPSHOT_WITH_DELETE) {
- Preconditions.checkState(isDeleteEnabled,
+
Preconditions.checkState(StringUtils.isNotEmpty(upsertConfig.getDeleteRecordColumn()),
"'deleteRecordColumn' must be provided with validDocIdsType: %s",
validDocIdsType);
}
return validDocIdsType;
diff --git
a/pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/test/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtilsTest.java
b/pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/test/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtilsTest.java
index 5abf106367f..0b55986df46 100644
---
a/pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/test/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtilsTest.java
+++
b/pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/test/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtilsTest.java
@@ -162,11 +162,12 @@ public class MinionTaskUtilsTest {
MinionTaskUtils.getValidDocIdsType(upsertConfig, taskConfigs,
UpsertCompactionTask.VALID_DOC_IDS_TYPE);
assertEquals(result1, ValidDocIdsType.SNAPSHOT);
- // Test 2: Default when delete is enabled
+ // Test 2: Default stays SNAPSHOT even when delete is enabled. Delete
records are retained until they expire via
+ // deletedKeysTTL rather than being dropped eagerly.
upsertConfig.setDeleteRecordColumn("deleted");
ValidDocIdsType result2 =
MinionTaskUtils.getValidDocIdsType(upsertConfig, taskConfigs,
UpsertCompactionTask.VALID_DOC_IDS_TYPE);
- assertEquals(result2, ValidDocIdsType.SNAPSHOT_WITH_DELETE);
+ assertEquals(result2, ValidDocIdsType.SNAPSHOT);
}
@Test
@@ -210,23 +211,24 @@ public class MinionTaskUtilsTest {
}
@Test
- public void testGetValidDocIdsTypeBackwardCompatibilityAndOverride() {
- // Test backward compatibility behavior when delete is enabled
+ public void testGetValidDocIdsTypeNotOverriddenForDeleteTable() {
+ // A user-specified validDocIdsType is always honored on a delete-enabled
table; it is never silently overridden
+ // to SNAPSHOT_WITH_DELETE.
UpsertConfig upsertConfig = new UpsertConfig();
upsertConfig.setDeleteRecordColumn("deleted");
Map<String, String> taskConfigs = new HashMap<>();
- // Test 1: SNAPSHOT gets overridden to SNAPSHOT_WITH_DELETE
+ // Test 1: SNAPSHOT is honored (not overridden to SNAPSHOT_WITH_DELETE)
taskConfigs.put(UpsertCompactionTask.VALID_DOC_IDS_TYPE, "SNAPSHOT");
ValidDocIdsType result1 =
MinionTaskUtils.getValidDocIdsType(upsertConfig, taskConfigs,
UpsertCompactionTask.VALID_DOC_IDS_TYPE);
- assertEquals(result1, ValidDocIdsType.SNAPSHOT_WITH_DELETE);
+ assertEquals(result1, ValidDocIdsType.SNAPSHOT);
- // Test 2: IN_MEMORY gets overridden to SNAPSHOT_WITH_DELETE
+ // Test 2: IN_MEMORY is honored (not overridden)
taskConfigs.put(UpsertCompactionTask.VALID_DOC_IDS_TYPE, "IN_MEMORY");
ValidDocIdsType result2 =
MinionTaskUtils.getValidDocIdsType(upsertConfig, taskConfigs,
UpsertCompactionTask.VALID_DOC_IDS_TYPE);
- assertEquals(result2, ValidDocIdsType.SNAPSHOT_WITH_DELETE);
+ assertEquals(result2, ValidDocIdsType.IN_MEMORY);
// Test 3: SNAPSHOT_WITH_DELETE stays the same
taskConfigs.put(UpsertCompactionTask.VALID_DOC_IDS_TYPE,
"SNAPSHOT_WITH_DELETE");
@@ -234,17 +236,17 @@ public class MinionTaskUtilsTest {
MinionTaskUtils.getValidDocIdsType(upsertConfig, taskConfigs,
UpsertCompactionTask.VALID_DOC_IDS_TYPE);
assertEquals(result3, ValidDocIdsType.SNAPSHOT_WITH_DELETE);
- // Test 4: IN_MEMORY_WITH_DELETE stays the same (not overridden)
+ // Test 4: IN_MEMORY_WITH_DELETE stays the same
taskConfigs.put(UpsertCompactionTask.VALID_DOC_IDS_TYPE,
"IN_MEMORY_WITH_DELETE");
ValidDocIdsType result4 =
MinionTaskUtils.getValidDocIdsType(upsertConfig, taskConfigs,
UpsertCompactionTask.VALID_DOC_IDS_TYPE);
assertEquals(result4, ValidDocIdsType.IN_MEMORY_WITH_DELETE);
- // Test 5: Case insensitive override behavior
- taskConfigs.put(UpsertCompactionTask.VALID_DOC_IDS_TYPE,
"in_memory_with_delete");
+ // Test 5: Case-insensitive parsing, still honored without override
+ taskConfigs.put(UpsertCompactionTask.VALID_DOC_IDS_TYPE, "snapshot");
ValidDocIdsType result5 =
MinionTaskUtils.getValidDocIdsType(upsertConfig, taskConfigs,
UpsertCompactionTask.VALID_DOC_IDS_TYPE);
- assertEquals(result5, ValidDocIdsType.IN_MEMORY_WITH_DELETE);
+ assertEquals(result5, ValidDocIdsType.SNAPSHOT);
}
@Test
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
