Copilot commented on code in PR #19115:
URL: https://github.com/apache/pinot/pull/19115#discussion_r3678057870
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -155,7 +156,10 @@ protected BasePartitionUpsertMetadataManager(String
tableNameWithType, int parti
_comparisonColumns = context.getComparisonColumns();
_deleteRecordColumn = context.getDeleteRecordColumn();
_hashFunction = context.getHashFunction();
- _partialUpsertHandler = context.getPartialUpsertHandler();
+ // Build a handler owned by this partition. PartialUpsertHandler is not
thread safe, and merges for a partition
+ // run on one consumer thread at a time, same as the _reusePreviousRow
scratch state used alongside it.
+ Supplier<PartialUpsertHandler> partialUpsertHandlerSupplier =
context.getPartialUpsertHandlerSupplier();
+ _partialUpsertHandler = partialUpsertHandlerSupplier != null ?
partialUpsertHandlerSupplier.get() : null;
Review Comment:
This change fixes a concurrency correctness issue by moving
PartialUpsertHandler ownership to the partition, but there is no regression
test asserting that two different partition metadata managers do not share the
same handler instance. Adding a deterministic unit test (no timing) that
creates two partition managers from the same UpsertContext and asserts the
supplier is invoked twice and yields distinct handlers would help prevent
accidental reintroduction of table-level sharing.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -155,7 +156,10 @@ protected BasePartitionUpsertMetadataManager(String
tableNameWithType, int parti
_comparisonColumns = context.getComparisonColumns();
_deleteRecordColumn = context.getDeleteRecordColumn();
_hashFunction = context.getHashFunction();
- _partialUpsertHandler = context.getPartialUpsertHandler();
+ // Build a handler owned by this partition. PartialUpsertHandler is not
thread safe, and merges for a partition
+ // run on one consumer thread at a time, same as the _reusePreviousRow
scratch state used alongside it.
+ Supplier<PartialUpsertHandler> partialUpsertHandlerSupplier =
context.getPartialUpsertHandlerSupplier();
+ _partialUpsertHandler = partialUpsertHandlerSupplier != null ?
partialUpsertHandlerSupplier.get() : null;
Review Comment:
If the context provides a non-null PartialUpsertHandler supplier but it
returns null, this partition manager ends up treating the table as FULL upsert
(because _partialUpsertHandler is null) while UpsertContext.getUpsertMode()
reports PARTIAL (because the supplier is non-null). That mismatch can silently
skip partial-upsert merging and lead to incorrect ingestion behavior. Consider
enforcing the contract that a non-null supplier must return a non-null handler.
--
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]
