Copilot commented on code in PR #17882:
URL: https://github.com/apache/pinot/pull/17882#discussion_r2935115296
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -398,10 +398,11 @@ protected void revertAndRemoveSegment(IndexSegment
segment,
_previousKeyToRecordLocationMap.remove(pk);
} else {
_logger.warn(
- "Consuming segment {} has already ingested the primary key
for docId {} from segment {}, suggesting"
+ "Consuming segment: {} has added the primary key for docId:
{} from the segment: {}, suggesting"
+ " that consumption is occurring concurrently with
segment replacement, which is undesirable "
- + "for consistency.",
recordLocation.getSegment().getSegmentName(), primaryKeyEntry.getKey(),
- segment.getSegmentName());
+ + "for consistency between replicas. Ensure
`isAllowPartialUpsertConsumptionDuringCommit` is "
+ + "set to DISALLOW_ALWAYS",
recordLocation.getSegment().getSegmentName(),
Review Comment:
This warn message mixes the deprecated boolean
`isAllowPartialUpsertConsumptionDuringCommit` with
`ParallelSegmentConsumptionPolicy`-style values (`DISALLOW_ALWAYS`). This is
likely confusing for operators because the flag can’t be set to that value.
Suggest rewriting to mention `ParallelSegmentConsumptionPolicy` explicitly
(e.g., recommend `DISALLOW_ALWAYS`/`ALLOW_DURING_BUILD_ONLY`), or keep the
deprecated flag and describe it as true/false. Also consider aligning the
wording with `ConcurrentMapPartitionUpsertMetadataManager` so both classes give
consistent guidance.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -265,10 +265,11 @@ protected void revertAndRemoveSegment(IndexSegment
segment,
_previousKeyToRecordLocationMap.remove(pk);
} else {
_logger.warn(
- "Consuming segment: {} has already ingested the primary key
for docId {} from segment: {}, suggesting"
+ "Consuming segment: {} has added the primary key for docId:
{} from the segment: {}, suggesting"
+ " that consumption is occurring concurrently with
segment replacement, which is undesirable "
- + "for consistency.",
recordLocation.getSegment().getSegmentName(), primaryKeyEntry.getKey(),
- segment.getSegmentName());
+ + "for consistency between replicas. Ensure
`isAllowPartialUpsertConsumptionDuringCommit` is "
+ + "not set to ALLOW_ALWAYS",
recordLocation.getSegment().getSegmentName(),
Review Comment:
The warning message references `isAllowPartialUpsertConsumptionDuringCommit`
but then suggests values like `ALLOW_ALWAYS`/`DISALLOW_ALWAYS`, which don’t
match the (deprecated) boolean meaning of that config. Consider updating the
log to reference `ParallelSegmentConsumptionPolicy` (and the relevant enum
values), or if you intend to keep the deprecated flag, phrase it in terms of a
boolean (e.g., set to true/false) so operators get actionable guidance.
--
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]
