xiangfu0 commented on code in PR #19998:
URL: https://github.com/apache/pinot/pull/19998#discussion_r4366238863
##########
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:
Fixed in 60040605ee. ImmutableSegmentImpl.getColumnNames() now uses
SegmentMetadataImpl.getAllColumns(), which reads mapped physical and virtual
column names without materializing a Schema. The mapped/schema name parity
assertion and focused tests passed.
##########
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:
Fixed the new skip behavior in 60040605ee: an updated segment still observed
while it is being untracked now caps the watermark under its read lock. If its
metadata is already destroyed, we persist the unset watermark so restart uses
the normal load path. The focused watermark test passed. A segment fully
untracked before the snapshot pass is already removed from this set by the
existing lifecycle code; this change does not alter that earlier behavior.
--
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]
