xiangfu0 commented on code in PR #18427:
URL: https://github.com/apache/pinot/pull/18427#discussion_r3195307998
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -537,11 +538,17 @@ protected boolean doAddRecord(MutableSegment segment,
RecordInfo recordInfo) {
}
} else {
// New primary key
+ isNewKey.set(true);
addDocId(segment, validDocIds, queryableDocIds, newDocId,
recordInfo);
return new RecordLocation(segment, newDocId, newComparisonValue,
1);
}
});
+ if (isNewKey.get()) {
Review Comment:
Same issue here: delete records are counted under the new insert/update
meters. In delete-enabled tables this misclassifies tombstone traffic as
inserts/updates, so the exposed per-table counters become incorrect even though
the underlying upsert state is handled correctly. This needs the same
`!recordInfo.isDeleteRecord()` guard or a dedicated delete counter.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -404,11 +405,17 @@ protected boolean doAddRecord(MutableSegment segment,
RecordInfo recordInfo) {
}
} else {
// New primary key
+ isNewKey.set(true);
addDocId(segment, validDocIds, queryableDocIds, newDocId,
recordInfo);
return new RecordLocation(segment, newDocId, newComparisonValue);
}
});
+ if (isNewKey.get()) {
Review Comment:
This block increments the new meters for every non-out-of-order record,
including tombstones. On delete-enabled upsert tables, a first-seen delete will
hit `UPSERT_NEW_KEYS_INSERTED` and a delete for an existing PK will hit
`UPSERT_EXISTING_KEYS_UPDATED`, even though neither is an insert/update row.
Because these are new exported Prometheus counters, that makes the public
metric contract wrong for delete-heavy workloads. Please guard these counters
with `!recordInfo.isDeleteRecord()` or add a separate delete meter.
--
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]
