You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "deepthi912 (via GitHub)" <gi...@apache.org> on 2026/03/14 02:11:10 UTC
[PR] Change the ERROR log into WARN log while reverting Upsert Metadata [pinot]
deepthi912 opened a new pull request, #17882:
URL: https://github.com/apache/pinot/pull/17882
- Added some more tests to make sure _previousKeyToRecordLocationMap is cleared after replacement and segment removal.
- Changed the ERROR log to WARN when two consuming segments are present.
- null check for previous record location makes sense only if the current segment for the key belongs to the consuming segment because the keys that point to current consuming segment's immutable segment will also have null previous record location once we replace those keys
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added more tests for #17503 [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17882:
URL: https://github.com/apache/pinot/pull/17882#issuecomment-4059338488
### :x: 1 Tests Failed:
| Tests completed | Failed | Passed | Skipped |
|---|---|---|---|
| 1428 | 1 | 1427 | 42 |
<details><summary>View the top 1 failed test(s) by shortest run time</summary>
> <pre><code class="language-python">org.apache.pinot.integration.tests.QueryQuotaClusterIntegrationTest::testLogicalTableWithDatabaseQueryQuota</code></pre>
> <details><summary>Stack Traces | 5.3s run time</summary>
>
> > <pre><code class="language-python">Expected nonzero failures for qps: 50 isLastFail: false did not expect [0] but found [0]</code></pre>
>
> </details>
</details>
To view more test analytics, go to the [Test Analytics Dashboard](https://app.codecov.io/gh/apache/pinot/tests/deepthi912%3Ainconsistent-fixes)
<sub>📋 Got 3 mins? [Take this short survey](https://forms.gle/22i53Qa1CySZjA6c7) to help us improve Test Analytics.</sub>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added more tests for #17503 [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added more tests for #17503 [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #17882:
URL: https://github.com/apache/pinot/pull/17882
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org