xiangfu0 commented on code in PR #19998:
URL: https://github.com/apache/pinot/pull/19998#discussion_r4366210162
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java:
##########
@@ -386,7 +476,19 @@ public SegmentVersion getVersion() {
@Override
public Schema getSchema() {
- return _schema;
+ if (_mappedStore == null) {
+ return _schema;
+ }
+ // A Schema has several maps and lists per segment. Construct it only for
callers that need it, without retaining
+ // another copy of the column directory for the lifetime of a wide segment.
+ Schema schema = new Schema();
Review Comment:
Small perf heads-up: for a mapped segment,
ImmutableSegmentImpl.getColumnNames() calls getSchema() which rebuilds a whole
Schema here on every call. SELECT * hits getColumnNames() once per segment per
query (SelectionOperatorUtils), so wide mapped segments re-materialize the
Schema just to read its name set. Might be worth a lighter name-only path.
Non-blocking since mapping is opt-in.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -1198,23 +1210,33 @@ protected void doTakeSnapshot(@Nullable
DocIdsSnapshot.Trigger trigger) {
/// the persisted watermark to keep such segments within TTL, and get them
added with the regular flow after restart.
/// The watermark in memory is not changed, and primary keys are still
expired against it, so a segment that keeps
/// missing its snapshot doesn't keep the expired primary keys in memory.
- private double getWatermarkToPersist() {
+ @VisibleForTesting
+ double getWatermarkToPersist() {
double watermark = _largestSeenComparisonValue.get();
if (_metadataTTL <= 0) {
return watermark;
}
double cappedWatermark = watermark;
- // Unlike taking the snapshots above, reading the max comparison value
doesn't need the segmentLock or the check
- // against _trackedSegments: it comes from the segment metadata, which can
still be read after the segment is
- // destroyed, and a segment removed in the meantime can only cap the
watermark lower than needed.
+ // A mapped segment's metadata is closed on destroy. Hold the segment read
lock while reading its maximum, and
+ // ignore stale entries that were removed from tracking after the snapshot
pass.
for (ImmutableSegmentImpl segment : _updatedSegmentsSinceLastSnapshot) {
- double maxComparisonValue = getMaxComparisonValue(segment);
- double maxWatermark = maxComparisonValue + _metadataTTL;
- // Make sure the segment is not out of TTL as checked by
isOutOfMetadataTTL(), regardless of rounding
- while (maxComparisonValue < maxWatermark - _metadataTTL) {
- maxWatermark = Math.nextDown(maxWatermark);
+ if (!tryAcquireSegmentReadLock(segment)) {
+ continue;
Review Comment:
This now skips a segment when its read lock can't be acquired (destroyed) or
it's no longer tracked, where the old code read the max even post-destroy.
Skipping means that segment no longer caps the watermark, so the persisted
value can end up higher and expire its keys sooner on restart. Is
skip-instead-of-cap the intended semantics for a removed segment? A one-line
note on why that's safe would help.
--
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]
