KKcorps commented on code in PR #19596:
URL: https://github.com/apache/pinot/pull/19596#discussion_r4143732353
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -426,15 +434,15 @@ protected GenericRow doUpdateRecord(GenericRow record,
RecordInfo recordInfo) {
if (!recordInfo.isDeleteRecord()
&&
recordInfo.getComparisonValue().compareTo(recordLocation.getComparisonValue())
>= 0) {
IndexSegment currentSegment = recordLocation.getSegment();
- ThreadSafeMutableRoaringBitmap currentQueryableDocIds =
currentSegment.getQueryableDocIds();
int currentDocId = recordLocation.getDocId();
- if (currentQueryableDocIds == null ||
currentQueryableDocIds.contains(currentDocId)) {
+ // Read lock: currentSegment cannot be destroyed while LazyRow
reads its columns. A consuming segment needs
+ // no lock: it is destroyed only after
replaceSegment()/removeSegment() has moved or dropped every location
+ // pointing at it, and those run under the same per-key compute as
this read.
+ if (tryAcquireSegmentReadLock(currentSegment)) {
Review Comment:
```
The real contention is with destroy() — it's a non-fair RRWL, so a queued
writer parks new readers while the consumer sits inside a computeIfPresent bin
lock. My harness doesn't produce that case yet.
```
this is what we need to test
also this perf number should be in terms of throughput imo and not latency
like this is the number of seconds we can support for partial upserts before
and after on 1 cpu core
--
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]
