klsince commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1711771750
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/UpsertContext.java:
##########
@@ -66,6 +69,8 @@ private UpsertContext(TableConfig tableConfig, Schema schema,
List<String> prima
_consistencyMode = consistencyMode;
_upsertViewRefreshIntervalMs = upsertViewRefreshIntervalMs;
_tableIndexDir = tableIndexDir;
+ _dropOutOfOrderRecord = dropOutOfOrderRecord;
Review Comment:
is this change related to this PR or a quick fix?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -858,6 +858,38 @@ static void validateUpsertAndDedupConfig(TableConfig
tableConfig, Schema schema)
}
}
+ // enableConsistentDeletes shouldn't exist with metadataTTL
+ if (upsertConfig != null && upsertConfig.isEnableConsistentDeletes()) {
Review Comment:
why not combine all the checks inside one if-check? as the if (condition) is
same
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/TableUpsertMetadataManagerFactory.java:
##########
@@ -83,8 +82,14 @@ public static TableUpsertMetadataManager create(TableConfig
tableConfig,
metadataManagerClass, tableNameWithType), e);
}
} else {
- LOGGER.info("Creating ConcurrentMapTableUpsertMetadataManager for table:
{}", tableNameWithType);
- metadataManager = new ConcurrentMapTableUpsertMetadataManager();
+ if (upsertConfig.isEnableConsistentDeletes()) {
Review Comment:
hmm.. if metadataManagerClass is set, the else-branch is skipped, so the
check of the new feature flag is skipped too.
so should we check the feature flag check inside
ConcurrentMapTableUpsertMetadataManager getOrCreatePartitionManager(), and
based on the flag, we return either the original partition mgr, or the new
partition mgr added in this PR.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
