KKcorps commented on code in PR #19596:
URL: https://github.com/apache/pinot/pull/19596#discussion_r4082528938
##########
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:
Have we considered the ingestion-throughput impact of taking this read lock
for every qualifying partial update whose previous row is in an immutable
segment? With a large active keyspace, especially just after segment rollover,
a substantial fraction of updates can take this path, so the lock/unlock and
`_destroyed` check become per-record overhead.
Do we have a before/after benchmark with most updates hitting immutable
predecessors, plus a mostly-mutable control, to quantify the throughput and CPU
impact? This seems like the main performance risk to validate for this change.
--
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]
