You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tibrewalpratik17 (via GitHub)" <gi...@apache.org> on 2024/06/10 10:08:01 UTC
[PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
tibrewalpratik17 opened a new pull request, #13347:
URL: https://github.com/apache/pinot/pull/13347
Labels:
`enhancement`
`upsert`
### Problem statement
In #12037, we added support for removing deleted keys and the validDocId for the deleted records after a TTL window. This ensured that deleted records would eventually be picked up by the compaction flow.
In #13342, we directly enabled compaction to take care of deleted rows.
In both scenarios, we could end up in a situation where an older non-deleted record for a particular key is not compacted, but the deleted record is. During a server restart, when all segments are loaded, Pinot would incorrectly mark the record as non-deleted and start returning it as a valid primary key, leading to an inconsistent state in the table.
**Example**:
Consider a primary key PK1 with records in segments S0 and S1. In S1, the record is marked as deleted. If S1 gets compacted but S0 doesn't due to threshold reasons in the upsert-compaction flow, during a server restart, the upsert-metadata-manager map would incorrectly point PK1 to S0, even though it should be considered deleted for the end-user.
**Why not use enablePreload?**
While enablePreload might prevent loading previous invalid records, it is not foolproof. Valid-doc-id snapshots are not always available due to scenarios like fetching segments from deepstore or upsert-compaction refreshing the segment. If a validDocID snapshot is missing and enablePreload is set to true, the segment loads normally, iterating through records and would potentially end-up reviving a deleted primary key with an old value with the present flow.
### Proposal
This PR aims to maintain the state of PK -> distinct-segment-count. This means tracking the number of segments in which a record exists for a given primary key. If the count is <= 1, we can compact the deleted record, ensuring that all other records in other segments are removed.
Changes required in the following upsert methods:
- `addOrReplaceSegment`: When adding a segment, increment the counter by 1. For replacing a segment, the removeSegment flow will decrement the count by 1.
- `removeSegment`: This method needs to decrement the count by 1 for all keys present in the segment, which requires more intrusive changes as we currently only iterate valid-doc-id records.
- `addRecord` : Here, we need to check if the last pointed record is from the same consuming segment and increment the value accordingly. This might need better checks with `dropOutOfOrder` config enabled.
**Inconsistent with enablePreload**
This proposal might not work with `enablePreload`, as `enablePreload` only iterates validDocIDs during segment-addition, ultimately missing counts for other invalidDocIDs PKs in the upsert-map.
We could enable this proposal behind a new config, `enableDeletionConsistencyWithCompaction`, and ensure that this config cannot be used simultaneously with `enablePreload`. Additionally, we can use this boolean flag in the `removeSegment` flow to decide whether to iterate the entire segment or just the valid-doc-ids.
cc @klsince @Jackie-Jiang raising this draft PR for early feedback on the proposal.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1701492112
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/TableUpsertMetadataManagerFactory.java:
##########
@@ -29,16 +29,15 @@
public class TableUpsertMetadataManagerFactory {
- private TableUpsertMetadataManagerFactory() {
- }
-
- private static final Logger LOGGER = LoggerFactory.getLogger(TableUpsertMetadataManagerFactory.class);
public static final String UPSERT_DEFAULT_METADATA_MANAGER_CLASS = "default.metadata.manager.class";
public static final String UPSERT_DEFAULT_ENABLE_SNAPSHOT = "default.enable.snapshot";
public static final String UPSERT_DEFAULT_ENABLE_PRELOAD = "default.enable.preload";
-
public static final String UPSERT_DEFAULT_ALLOW_PARTIAL_UPSERT_CONSUMPTION_DURING_COMMIT =
"default.allow.partial.upsert.consumption.during.commit";
+ private static final Logger LOGGER = LoggerFactory.getLogger(TableUpsertMetadataManagerFactory.class);
Review Comment:
answered here - https://github.com/apache/pinot/pull/13347/files#r1701487051
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1683107875
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,373 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'consistentDeletes' set to true.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
Review Comment:
> What's the main reason for a new metadata manager? Is it because we are adding a new int to RecordLocation, and there are perf concerns?
Yes the main reason is using a new int to RecordLocation which we discussed will add 16bytes per key(unsigned) and for supporting 100M per host can easily add 1600MB. We thought of making RecordLocation an interface as well to achieve this but then the metadataManager class will become a bit incomprehensible with lots of if-else's.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana merged PR #13347:
URL: https://github.com/apache/pinot/pull/13347
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1701502439
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,373 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'consistentDeletes' set to true.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ currentDistinctSegmentCount + 1);
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ currentDistinctSegmentCount + 1);
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
Review Comment:
Good point!
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1711771750
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/UpsertContext.java:
##########
@@ -66,6 +69,8 @@ private UpsertContext(TableConfig tableConfig, Schema schema, List<String> prima
_consistencyMode = consistencyMode;
_upsertViewRefreshIntervalMs = upsertViewRefreshIntervalMs;
_tableIndexDir = tableIndexDir;
+ _dropOutOfOrderRecord = dropOutOfOrderRecord;
Review Comment:
is this change related to this PR or a quick fix?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -858,6 +858,38 @@ static void validateUpsertAndDedupConfig(TableConfig tableConfig, Schema schema)
}
}
+ // enableConsistentDeletes shouldn't exist with metadataTTL
+ if (upsertConfig != null && upsertConfig.isEnableConsistentDeletes()) {
Review Comment:
why not combine all the checks inside one if-check? as the if (condition) is same
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/TableUpsertMetadataManagerFactory.java:
##########
@@ -83,8 +82,14 @@ public static TableUpsertMetadataManager create(TableConfig tableConfig,
metadataManagerClass, tableNameWithType), e);
}
} else {
- LOGGER.info("Creating ConcurrentMapTableUpsertMetadataManager for table: {}", tableNameWithType);
- metadataManager = new ConcurrentMapTableUpsertMetadataManager();
+ if (upsertConfig.isEnableConsistentDeletes()) {
Review Comment:
hmm.. if metadataManagerClass is set, the else-branch is skipped, so the check of the new feature flag is skipped too.
so should we check the feature flag check inside ConcurrentMapTableUpsertMetadataManager getOrCreatePartitionManager(), and based on the flag, we return either the original partition mgr, or the new partition mgr added in this PR.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1710012624
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapTableUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.pinot.segment.spi.SegmentContext;
+
+
+/**
+ * Implementation of {@link TableUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'consistentDeletes' set to true.
+ */
+public class ConcurrentMapTableUpsertMetadataManagerForConsistentDeletes extends BaseTableUpsertMetadataManager {
Review Comment:
Other than `doUpdateRecord` and `getNumPrimaryKeys` none of the methods are same afaik.. since RecordLocation is a static class in each manager it makes it a bit unreadable when creating new RecordLocation in this class..
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1683111727
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BaseTableUpsertMetadataManager.java:
##########
@@ -81,6 +81,8 @@ public void init(TableConfig tableConfig, Schema schema, TableDataManager tableD
.setPartialUpsertHandler(partialUpsertHandler).setEnableSnapshot(enableSnapshot).setEnablePreload(enablePreload)
.setMetadataTTL(metadataTTL).setDeletedKeysTTL(deletedKeysTTL).setConsistencyMode(_consistencyMode)
.setUpsertViewRefreshIntervalMs(upsertViewRefreshIntervalMs).setTableIndexDir(tableIndexDir)
+ .setDropOutOfOrderRecord(upsertConfig.isDropOutOfOrderRecord())
+ .setEnableConsistentDeletes(upsertConfig.isEnableConsistentDeletes())
Review Comment:
So folks can enable `deletedKeysTTL` without enabling compaction as well. This config is only required when you are enabling deletedKeysTTL + compaction as this entire PR ensures data consistency for this specific scenario.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1709966912
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapTableUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.pinot.segment.spi.SegmentContext;
+
+
+/**
+ * Implementation of {@link TableUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'consistentDeletes' set to true.
+ */
+public class ConcurrentMapTableUpsertMetadataManagerForConsistentDeletes extends BaseTableUpsertMetadataManager {
Review Comment:
Consider to extract a base class?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,385 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'enableConsistentDeletes' set to true.
+ *
+ * Consistent deletion ensures that when deletedKeysTTL is enabled with UpsertCompaction, the key metadata is
+ * removed from the HashMap only after all other records in the old segments are compacted. This guarantees
+ * data consistency. Without this, there can be a scenario where a deleted record is compacted first, while an
+ * old record remains non-compacted in a previous segment. During a server restart, this could lead to the old
+ * record reappearing. For the end-user, this would result in a data loss or inconsistency scenario, as the
+ * record was marked for deletion.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ throw new UnsupportedOperationException("Consistent-deletion does not support preloading of segments.");
+ }
+
+ @Override
+ protected void doRemoveSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing {} segment: {}, current primary key count: {}",
+ segment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName, getNumPrimaryKeys());
+ long startTimeMs = System.currentTimeMillis();
+
+ try (
+ UpsertUtils.PrimaryKeyReader primaryKeyReader = new UpsertUtils.PrimaryKeyReader(segment, _primaryKeyColumns)) {
+ removeSegment(segment,
+ UpsertUtils.getPrimaryKeyIterator(primaryKeyReader, segment.getSegmentMetadata().getTotalDocs()));
+ } catch (Exception e) {
+ throw new RuntimeException(
+ String.format("Caught exception while removing segment: %s, table: %s", segment.getSegmentName(),
+ _tableNameWithType), e);
+ }
+
+ // Update metrics
+ long numPrimaryKeys = getNumPrimaryKeys();
+ updatePrimaryKeyGauge(numPrimaryKeys);
+ _logger.info("Finished removing segment: {} in {}ms, current primary key count: {}", segmentName,
+ System.currentTimeMillis() - startTimeMs, numPrimaryKeys);
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, Iterator<PrimaryKey> primaryKeyIterator) {
+ Set<Object> deDuplicatedPrimaryKeys = new HashSet<>();
+ while (primaryKeyIterator.hasNext()) {
+ PrimaryKey primaryKey = primaryKeyIterator.next();
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(primaryKey, _hashFunction),
+ (pk, recordLocation) -> {
+ if (deDuplicatedPrimaryKeys.contains(pk)) {
+ return recordLocation;
+ }
+ deDuplicatedPrimaryKeys.add(pk);
+ if (recordLocation.getSegment() == segment) {
+ return null;
+ }
+ return new RecordLocation(recordLocation.getSegment(), recordLocation.getDocId(),
+ recordLocation.getComparisonValue(),
+ RecordLocation.decrementSegmentCount(recordLocation.getDistinctSegmentCount()));
+ });
+ }
+ }
+
+ @Override
+ public void doRemoveExpiredPrimaryKeys() {
+ AtomicInteger numDeletedTTLKeysRemoved = new AtomicInteger();
+ double largestSeenComparisonValue = _largestSeenComparisonValue.get();
+ double deletedKeysThreshold;
+ if (_deletedKeysTTL > 0) {
+ deletedKeysThreshold = largestSeenComparisonValue - _deletedKeysTTL;
+ } else {
+ deletedKeysThreshold = Double.MIN_VALUE;
+ }
+
+ _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
+ double comparisonValue = ((Number) recordLocation.getComparisonValue()).doubleValue();
+ if (_deletedKeysTTL > 0 && comparisonValue < deletedKeysThreshold
+ && recordLocation.getDistinctSegmentCount() <= 1) {
Review Comment:
perhaps add some comments on this check, or add comment on the disctinctSegmentCount field in recordLocation that it's used here, as it's kinda critical for the correctness of this feature.
also looks like a lot of methods are same with ConcurrentMapPartitionUpsertMetadataManager. How about extracting a base class like BaseConcurrentMapPartitionUpsertMetadataManager, for both to reuse.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1712724349
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,391 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'enableConsistentDeletes' set to true.
+ *
+ * Consistent deletion ensures that when deletedKeysTTL is enabled with UpsertCompaction, the key metadata is
+ * removed from the HashMap only after all other records in the old segments are compacted. This guarantees
+ * data consistency. Without this, there can be a scenario where a deleted record is compacted first, while an
+ * old record remains non-compacted in a previous segment. During a server restart, this could lead to the old
+ * record reappearing. For the end-user, this would result in a data loss or inconsistency scenario, as the
+ * record was marked for deletion.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ throw new UnsupportedOperationException("Consistent-deletion does not support preloading of segments.");
+ }
+
+ @Override
+ protected void doRemoveSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing {} segment: {}, current primary key count: {}",
+ segment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName, getNumPrimaryKeys());
+ long startTimeMs = System.currentTimeMillis();
+
+ try (
+ UpsertUtils.PrimaryKeyReader primaryKeyReader = new UpsertUtils.PrimaryKeyReader(segment, _primaryKeyColumns)) {
+ removeSegment(segment,
+ UpsertUtils.getPrimaryKeyIterator(primaryKeyReader, segment.getSegmentMetadata().getTotalDocs()));
+ } catch (Exception e) {
+ throw new RuntimeException(
+ String.format("Caught exception while removing segment: %s, table: %s", segment.getSegmentName(),
+ _tableNameWithType), e);
+ }
+
+ // Update metrics
+ long numPrimaryKeys = getNumPrimaryKeys();
+ updatePrimaryKeyGauge(numPrimaryKeys);
+ _logger.info("Finished removing segment: {} in {}ms, current primary key count: {}", segmentName,
+ System.currentTimeMillis() - startTimeMs, numPrimaryKeys);
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, Iterator<PrimaryKey> primaryKeyIterator) {
+ Set<Object> deDuplicatedPrimaryKeys = new HashSet<>();
+ while (primaryKeyIterator.hasNext()) {
+ PrimaryKey primaryKey = primaryKeyIterator.next();
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(primaryKey, _hashFunction),
+ (pk, recordLocation) -> {
+ if (deDuplicatedPrimaryKeys.contains(pk)) {
+ return recordLocation;
+ }
+ deDuplicatedPrimaryKeys.add(pk);
+ if (recordLocation.getSegment() == segment) {
+ return null;
+ }
+ return new RecordLocation(recordLocation.getSegment(), recordLocation.getDocId(),
+ recordLocation.getComparisonValue(),
+ RecordLocation.decrementSegmentCount(recordLocation.getDistinctSegmentCount()));
+ });
+ }
+ }
+
+ @Override
+ public void doRemoveExpiredPrimaryKeys() {
+ AtomicInteger numDeletedTTLKeysRemoved = new AtomicInteger();
+ double largestSeenComparisonValue = _largestSeenComparisonValue.get();
+ double deletedKeysThreshold;
+ if (_deletedKeysTTL > 0) {
+ deletedKeysThreshold = largestSeenComparisonValue - _deletedKeysTTL;
+ } else {
+ deletedKeysThreshold = Double.MIN_VALUE;
+ }
+
+ _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
+ double comparisonValue = ((Number) recordLocation.getComparisonValue()).doubleValue();
+ // We need to verify that the record belongs to only one segment. If a record is part of multiple segments,
+ // an issue can arise where the upsert compaction might first process the segment containing the delete record
+ // while the previous segment(s) are not compacted. Upon restart, this can inadvertently revive the key
+ // that was originally marked for deletion.
+ if (_deletedKeysTTL > 0 && comparisonValue < deletedKeysThreshold
+ && recordLocation.getDistinctSegmentCount() <= 1) {
+ ThreadSafeMutableRoaringBitmap currentQueryableDocIds = recordLocation.getSegment().getQueryableDocIds();
+ // if key not part of queryable doc id, it means it is deleted
+ if (currentQueryableDocIds != null && !currentQueryableDocIds.contains(recordLocation.getDocId())) {
+ _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ removeDocId(recordLocation.getSegment(), recordLocation.getDocId());
+ numDeletedTTLKeysRemoved.getAndIncrement();
+ }
+ }
+ });
+
+ // Update metrics
+ updatePrimaryKeyGauge();
+ int numDeletedTTLKeys = numDeletedTTLKeysRemoved.get();
+ if (numDeletedTTLKeys > 0) {
+ _logger.info("Deleted {} primary keys based on deletedKeysTTL", numDeletedTTLKeys);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.DELETED_KEYS_TTL_PRIMARY_KEYS_REMOVED,
+ numDeletedTTLKeys);
+ }
+ }
+
+ @Override
+ protected boolean doAddRecord(MutableSegment segment, RecordInfo recordInfo) {
+ AtomicBoolean isOutOfOrderRecord = new AtomicBoolean(false);
+ ThreadSafeMutableRoaringBitmap validDocIds = Objects.requireNonNull(segment.getValidDocIds());
+ ThreadSafeMutableRoaringBitmap queryableDocIds = segment.getQueryableDocIds();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+
+ // When TTL is enabled, update largestSeenComparisonValue when adding new record
+ if (_deletedKeysTTL > 0) {
+ double comparisonValue = ((Number) newComparisonValue).doubleValue();
+ _largestSeenComparisonValue.getAndUpdate(v -> Math.max(v, comparisonValue));
+ }
+
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ // Update the record location when the new comparison value is greater than or equal to the current value.
+ // Update the record location when there is a tie to keep the newer record.
+ if (newComparisonValue.compareTo(currentRecordLocation.getComparisonValue()) >= 0) {
+ int currentDocId = currentRecordLocation.getDocId();
+ if (segment == currentSegment) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ currentRecordLocation.getDistinctSegmentCount());
+ } else {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentRecordLocation.getDistinctSegmentCount()));
+ }
+ } else {
+ // Out-of-order record
+ handleOutOfOrderEvent(currentRecordLocation.getComparisonValue(), recordInfo.getComparisonValue());
+ isOutOfOrderRecord.set(true);
+ if (segment == currentSegment) {
+ return currentRecordLocation;
+ } else {
+ return new RecordLocation(currentSegment, currentRecordLocation.getDocId(),
+ currentRecordLocation.getComparisonValue(),
+ _context.isDropOutOfOrderRecord() ? currentRecordLocation.getDistinctSegmentCount()
+ : RecordLocation.incrementSegmentCount(currentRecordLocation.getDistinctSegmentCount()));
+ }
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+
+ updatePrimaryKeyGauge();
+ return !isOutOfOrderRecord.get();
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, MutableRoaringBitmap validDocIds) {
+ }
+
+ @Override
+ protected GenericRow doUpdateRecord(GenericRow record, RecordInfo recordInfo) {
+ assert _partialUpsertHandler != null;
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (pk, recordLocation) -> {
+ // Read the previous record if the following conditions are met:
+ // - New record is not a DELETE record
+ // - New record is not out-of-order
+ // - Previous record is not deleted
+ 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)) {
+ _reusePreviousRow.init(currentSegment, currentDocId);
+ _partialUpsertHandler.merge(_reusePreviousRow, record, _reuseMergeResultHolder);
+ _reuseMergeResultHolder.clear();
+ }
+ }
+ return recordLocation;
+ });
+ return record;
+ }
+
+ @VisibleForTesting
+ static class RecordLocation {
+ private final IndexSegment _segment;
+ private final int _docId;
+ private final Comparable _comparisonValue;
+ // The number of distinct segments in which the record is present. If this count is less than or equal to 1,
+ // we proceed to remove the record from the primary hashmap during the deletedKeysTTL process.
+ private final int _distinctSegmentCount;
+
+ public RecordLocation(IndexSegment indexSegment, int docId, Comparable comparisonValue, int distinctSegmentCount) {
+ _segment = indexSegment;
+ _docId = docId;
+ _comparisonValue = comparisonValue;
+ _distinctSegmentCount = distinctSegmentCount;
+ }
+
+ public static int incrementSegmentCount(int count) {
+ return count + 1;
+ }
+
+ public static int decrementSegmentCount(int count) {
+ return count - 1;
+ }
Review Comment:
Hmm are you suggesting one helper method or no helper method at all and do +1, -1 inplace?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1711817991
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/UpsertContext.java:
##########
@@ -66,6 +69,8 @@ private UpsertContext(TableConfig tableConfig, Schema schema, List<String> prima
_consistencyMode = consistencyMode;
_upsertViewRefreshIntervalMs = upsertViewRefreshIntervalMs;
_tableIndexDir = tableIndexDir;
+ _dropOutOfOrderRecord = dropOutOfOrderRecord;
Review Comment:
this was added on purpose. we are using this in `#addRecord` method to see if we are adding an out-of-order event in the segment or not and accordingly update the distinct-segment-count value.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1714386078
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/UpsertConfig.java:
##########
@@ -79,6 +79,9 @@ public enum ConsistencyMode {
@JsonPropertyDescription("Whether to preload segments for fast upsert metadata recovery")
private boolean _enablePreload;
+ @JsonPropertyDescription("If we are using deletion + compaction we need to enable this for data consistency")
Review Comment:
Have you pushed the 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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #13347:
URL: https://github.com/apache/pinot/pull/13347#issuecomment-2292491838
Good job! Can you also help update the pinot doc to reflect this new feature?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1638652782
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,348 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'consistentDeletes' set to true.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void addOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ if (_partialUpsertHandler != null) {
+ recordInfoIterator = resolveComparisonTies(recordInfoIterator, _hashFunction);
+ }
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ currentDistinctSegmentCount + 1);
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ currentDistinctSegmentCount + 1);
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ _primaryKeyToRecordLocationMap.put(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ new RecordLocation(segment, newDocId, newComparisonValue, 1));
+ }
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, MutableRoaringBitmap validDocIds) {
+ try (UpsertUtils.RecordInfoReader recordInfoReader = new UpsertUtils.RecordInfoReader(segment, _primaryKeyColumns,
+ _comparisonColumns, _deleteRecordColumn)) {
+ Iterator<RecordInfo> iterator =
+ UpsertUtils.getRecordInfoIterator(recordInfoReader, segment.getSegmentMetadata().getTotalDocs());
+ while (iterator.hasNext()) {
+ RecordInfo recordInfo = iterator.next();
+ _primaryKeyToRecordLocationMap.computeIfPresent(
+ HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction), (pk, recordLocation) -> {
Review Comment:
self-review: process one pk strictly-once during segment removal here.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1683111727
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BaseTableUpsertMetadataManager.java:
##########
@@ -81,6 +81,8 @@ public void init(TableConfig tableConfig, Schema schema, TableDataManager tableD
.setPartialUpsertHandler(partialUpsertHandler).setEnableSnapshot(enableSnapshot).setEnablePreload(enablePreload)
.setMetadataTTL(metadataTTL).setDeletedKeysTTL(deletedKeysTTL).setConsistencyMode(_consistencyMode)
.setUpsertViewRefreshIntervalMs(upsertViewRefreshIntervalMs).setTableIndexDir(tableIndexDir)
+ .setDropOutOfOrderRecord(upsertConfig.isDropOutOfOrderRecord())
+ .setEnableConsistentDeletes(upsertConfig.isEnableConsistentDeletes())
Review Comment:
So folks can enable `deletedKeysTTL` without enabling compaction or vice versa. This config is only required when you are enabling deletedKeysTTL + compaction as this entire PR ensures data consistency for this specific scenario.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1712425074
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,391 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'enableConsistentDeletes' set to true.
+ *
+ * Consistent deletion ensures that when deletedKeysTTL is enabled with UpsertCompaction, the key metadata is
+ * removed from the HashMap only after all other records in the old segments are compacted. This guarantees
+ * data consistency. Without this, there can be a scenario where a deleted record is compacted first, while an
+ * old record remains non-compacted in a previous segment. During a server restart, this could lead to the old
+ * record reappearing. For the end-user, this would result in a data loss or inconsistency scenario, as the
+ * record was marked for deletion.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ throw new UnsupportedOperationException("Consistent-deletion does not support preloading of segments.");
+ }
+
+ @Override
+ protected void doRemoveSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing {} segment: {}, current primary key count: {}",
+ segment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName, getNumPrimaryKeys());
+ long startTimeMs = System.currentTimeMillis();
+
+ try (
+ UpsertUtils.PrimaryKeyReader primaryKeyReader = new UpsertUtils.PrimaryKeyReader(segment, _primaryKeyColumns)) {
+ removeSegment(segment,
+ UpsertUtils.getPrimaryKeyIterator(primaryKeyReader, segment.getSegmentMetadata().getTotalDocs()));
+ } catch (Exception e) {
+ throw new RuntimeException(
+ String.format("Caught exception while removing segment: %s, table: %s", segment.getSegmentName(),
+ _tableNameWithType), e);
+ }
+
+ // Update metrics
+ long numPrimaryKeys = getNumPrimaryKeys();
+ updatePrimaryKeyGauge(numPrimaryKeys);
+ _logger.info("Finished removing segment: {} in {}ms, current primary key count: {}", segmentName,
+ System.currentTimeMillis() - startTimeMs, numPrimaryKeys);
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, Iterator<PrimaryKey> primaryKeyIterator) {
+ Set<Object> deDuplicatedPrimaryKeys = new HashSet<>();
+ while (primaryKeyIterator.hasNext()) {
+ PrimaryKey primaryKey = primaryKeyIterator.next();
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(primaryKey, _hashFunction),
+ (pk, recordLocation) -> {
+ if (deDuplicatedPrimaryKeys.contains(pk)) {
+ return recordLocation;
+ }
+ deDuplicatedPrimaryKeys.add(pk);
+ if (recordLocation.getSegment() == segment) {
+ return null;
+ }
+ return new RecordLocation(recordLocation.getSegment(), recordLocation.getDocId(),
+ recordLocation.getComparisonValue(),
+ RecordLocation.decrementSegmentCount(recordLocation.getDistinctSegmentCount()));
+ });
+ }
+ }
+
+ @Override
+ public void doRemoveExpiredPrimaryKeys() {
+ AtomicInteger numDeletedTTLKeysRemoved = new AtomicInteger();
+ double largestSeenComparisonValue = _largestSeenComparisonValue.get();
+ double deletedKeysThreshold;
+ if (_deletedKeysTTL > 0) {
+ deletedKeysThreshold = largestSeenComparisonValue - _deletedKeysTTL;
+ } else {
+ deletedKeysThreshold = Double.MIN_VALUE;
+ }
+
+ _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
+ double comparisonValue = ((Number) recordLocation.getComparisonValue()).doubleValue();
+ // We need to verify that the record belongs to only one segment. If a record is part of multiple segments,
+ // an issue can arise where the upsert compaction might first process the segment containing the delete record
+ // while the previous segment(s) are not compacted. Upon restart, this can inadvertently revive the key
+ // that was originally marked for deletion.
+ if (_deletedKeysTTL > 0 && comparisonValue < deletedKeysThreshold
+ && recordLocation.getDistinctSegmentCount() <= 1) {
+ ThreadSafeMutableRoaringBitmap currentQueryableDocIds = recordLocation.getSegment().getQueryableDocIds();
+ // if key not part of queryable doc id, it means it is deleted
+ if (currentQueryableDocIds != null && !currentQueryableDocIds.contains(recordLocation.getDocId())) {
+ _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ removeDocId(recordLocation.getSegment(), recordLocation.getDocId());
+ numDeletedTTLKeysRemoved.getAndIncrement();
+ }
+ }
+ });
+
+ // Update metrics
+ updatePrimaryKeyGauge();
+ int numDeletedTTLKeys = numDeletedTTLKeysRemoved.get();
+ if (numDeletedTTLKeys > 0) {
+ _logger.info("Deleted {} primary keys based on deletedKeysTTL", numDeletedTTLKeys);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.DELETED_KEYS_TTL_PRIMARY_KEYS_REMOVED,
+ numDeletedTTLKeys);
+ }
+ }
+
+ @Override
+ protected boolean doAddRecord(MutableSegment segment, RecordInfo recordInfo) {
+ AtomicBoolean isOutOfOrderRecord = new AtomicBoolean(false);
+ ThreadSafeMutableRoaringBitmap validDocIds = Objects.requireNonNull(segment.getValidDocIds());
+ ThreadSafeMutableRoaringBitmap queryableDocIds = segment.getQueryableDocIds();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+
+ // When TTL is enabled, update largestSeenComparisonValue when adding new record
+ if (_deletedKeysTTL > 0) {
+ double comparisonValue = ((Number) newComparisonValue).doubleValue();
+ _largestSeenComparisonValue.getAndUpdate(v -> Math.max(v, comparisonValue));
+ }
+
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ // Update the record location when the new comparison value is greater than or equal to the current value.
+ // Update the record location when there is a tie to keep the newer record.
+ if (newComparisonValue.compareTo(currentRecordLocation.getComparisonValue()) >= 0) {
+ int currentDocId = currentRecordLocation.getDocId();
+ if (segment == currentSegment) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ currentRecordLocation.getDistinctSegmentCount());
+ } else {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentRecordLocation.getDistinctSegmentCount()));
+ }
+ } else {
+ // Out-of-order record
+ handleOutOfOrderEvent(currentRecordLocation.getComparisonValue(), recordInfo.getComparisonValue());
+ isOutOfOrderRecord.set(true);
+ if (segment == currentSegment) {
+ return currentRecordLocation;
+ } else {
+ return new RecordLocation(currentSegment, currentRecordLocation.getDocId(),
+ currentRecordLocation.getComparisonValue(),
+ _context.isDropOutOfOrderRecord() ? currentRecordLocation.getDistinctSegmentCount()
+ : RecordLocation.incrementSegmentCount(currentRecordLocation.getDistinctSegmentCount()));
+ }
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+
+ updatePrimaryKeyGauge();
+ return !isOutOfOrderRecord.get();
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, MutableRoaringBitmap validDocIds) {
Review Comment:
(minor) Do you need to override this? Making it no-op is confusing
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,391 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'enableConsistentDeletes' set to true.
+ *
+ * Consistent deletion ensures that when deletedKeysTTL is enabled with UpsertCompaction, the key metadata is
+ * removed from the HashMap only after all other records in the old segments are compacted. This guarantees
+ * data consistency. Without this, there can be a scenario where a deleted record is compacted first, while an
+ * old record remains non-compacted in a previous segment. During a server restart, this could lead to the old
+ * record reappearing. For the end-user, this would result in a data loss or inconsistency scenario, as the
+ * record was marked for deletion.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ throw new UnsupportedOperationException("Consistent-deletion does not support preloading of segments.");
+ }
+
+ @Override
+ protected void doRemoveSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing {} segment: {}, current primary key count: {}",
+ segment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName, getNumPrimaryKeys());
+ long startTimeMs = System.currentTimeMillis();
+
+ try (
+ UpsertUtils.PrimaryKeyReader primaryKeyReader = new UpsertUtils.PrimaryKeyReader(segment, _primaryKeyColumns)) {
+ removeSegment(segment,
+ UpsertUtils.getPrimaryKeyIterator(primaryKeyReader, segment.getSegmentMetadata().getTotalDocs()));
+ } catch (Exception e) {
+ throw new RuntimeException(
+ String.format("Caught exception while removing segment: %s, table: %s", segment.getSegmentName(),
+ _tableNameWithType), e);
+ }
+
+ // Update metrics
+ long numPrimaryKeys = getNumPrimaryKeys();
+ updatePrimaryKeyGauge(numPrimaryKeys);
+ _logger.info("Finished removing segment: {} in {}ms, current primary key count: {}", segmentName,
+ System.currentTimeMillis() - startTimeMs, numPrimaryKeys);
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, Iterator<PrimaryKey> primaryKeyIterator) {
+ Set<Object> deDuplicatedPrimaryKeys = new HashSet<>();
+ while (primaryKeyIterator.hasNext()) {
+ PrimaryKey primaryKey = primaryKeyIterator.next();
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(primaryKey, _hashFunction),
+ (pk, recordLocation) -> {
+ if (deDuplicatedPrimaryKeys.contains(pk)) {
+ return recordLocation;
+ }
+ deDuplicatedPrimaryKeys.add(pk);
+ if (recordLocation.getSegment() == segment) {
+ return null;
+ }
Review Comment:
(minor) Consider moving this check before unique key check to reduce key set size
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,391 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'enableConsistentDeletes' set to true.
+ *
+ * Consistent deletion ensures that when deletedKeysTTL is enabled with UpsertCompaction, the key metadata is
+ * removed from the HashMap only after all other records in the old segments are compacted. This guarantees
+ * data consistency. Without this, there can be a scenario where a deleted record is compacted first, while an
+ * old record remains non-compacted in a previous segment. During a server restart, this could lead to the old
+ * record reappearing. For the end-user, this would result in a data loss or inconsistency scenario, as the
+ * record was marked for deletion.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ throw new UnsupportedOperationException("Consistent-deletion does not support preloading of segments.");
+ }
+
+ @Override
+ protected void doRemoveSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing {} segment: {}, current primary key count: {}",
+ segment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName, getNumPrimaryKeys());
+ long startTimeMs = System.currentTimeMillis();
+
+ try (
+ UpsertUtils.PrimaryKeyReader primaryKeyReader = new UpsertUtils.PrimaryKeyReader(segment, _primaryKeyColumns)) {
+ removeSegment(segment,
+ UpsertUtils.getPrimaryKeyIterator(primaryKeyReader, segment.getSegmentMetadata().getTotalDocs()));
+ } catch (Exception e) {
+ throw new RuntimeException(
+ String.format("Caught exception while removing segment: %s, table: %s", segment.getSegmentName(),
+ _tableNameWithType), e);
+ }
+
+ // Update metrics
+ long numPrimaryKeys = getNumPrimaryKeys();
+ updatePrimaryKeyGauge(numPrimaryKeys);
+ _logger.info("Finished removing segment: {} in {}ms, current primary key count: {}", segmentName,
+ System.currentTimeMillis() - startTimeMs, numPrimaryKeys);
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, Iterator<PrimaryKey> primaryKeyIterator) {
+ Set<Object> deDuplicatedPrimaryKeys = new HashSet<>();
+ while (primaryKeyIterator.hasNext()) {
+ PrimaryKey primaryKey = primaryKeyIterator.next();
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(primaryKey, _hashFunction),
+ (pk, recordLocation) -> {
+ if (deDuplicatedPrimaryKeys.contains(pk)) {
+ return recordLocation;
+ }
+ deDuplicatedPrimaryKeys.add(pk);
+ if (recordLocation.getSegment() == segment) {
+ return null;
+ }
+ return new RecordLocation(recordLocation.getSegment(), recordLocation.getDocId(),
+ recordLocation.getComparisonValue(),
+ RecordLocation.decrementSegmentCount(recordLocation.getDistinctSegmentCount()));
+ });
+ }
+ }
+
+ @Override
+ public void doRemoveExpiredPrimaryKeys() {
+ AtomicInteger numDeletedTTLKeysRemoved = new AtomicInteger();
+ double largestSeenComparisonValue = _largestSeenComparisonValue.get();
+ double deletedKeysThreshold;
+ if (_deletedKeysTTL > 0) {
+ deletedKeysThreshold = largestSeenComparisonValue - _deletedKeysTTL;
+ } else {
+ deletedKeysThreshold = Double.MIN_VALUE;
+ }
+
+ _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
+ double comparisonValue = ((Number) recordLocation.getComparisonValue()).doubleValue();
+ // We need to verify that the record belongs to only one segment. If a record is part of multiple segments,
+ // an issue can arise where the upsert compaction might first process the segment containing the delete record
+ // while the previous segment(s) are not compacted. Upon restart, this can inadvertently revive the key
+ // that was originally marked for deletion.
+ if (_deletedKeysTTL > 0 && comparisonValue < deletedKeysThreshold
+ && recordLocation.getDistinctSegmentCount() <= 1) {
+ ThreadSafeMutableRoaringBitmap currentQueryableDocIds = recordLocation.getSegment().getQueryableDocIds();
+ // if key not part of queryable doc id, it means it is deleted
+ if (currentQueryableDocIds != null && !currentQueryableDocIds.contains(recordLocation.getDocId())) {
+ _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ removeDocId(recordLocation.getSegment(), recordLocation.getDocId());
+ numDeletedTTLKeysRemoved.getAndIncrement();
+ }
+ }
+ });
+
+ // Update metrics
+ updatePrimaryKeyGauge();
+ int numDeletedTTLKeys = numDeletedTTLKeysRemoved.get();
+ if (numDeletedTTLKeys > 0) {
+ _logger.info("Deleted {} primary keys based on deletedKeysTTL", numDeletedTTLKeys);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.DELETED_KEYS_TTL_PRIMARY_KEYS_REMOVED,
+ numDeletedTTLKeys);
+ }
+ }
+
+ @Override
+ protected boolean doAddRecord(MutableSegment segment, RecordInfo recordInfo) {
+ AtomicBoolean isOutOfOrderRecord = new AtomicBoolean(false);
+ ThreadSafeMutableRoaringBitmap validDocIds = Objects.requireNonNull(segment.getValidDocIds());
+ ThreadSafeMutableRoaringBitmap queryableDocIds = segment.getQueryableDocIds();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+
+ // When TTL is enabled, update largestSeenComparisonValue when adding new record
+ if (_deletedKeysTTL > 0) {
+ double comparisonValue = ((Number) newComparisonValue).doubleValue();
+ _largestSeenComparisonValue.getAndUpdate(v -> Math.max(v, comparisonValue));
+ }
+
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ // Update the record location when the new comparison value is greater than or equal to the current value.
+ // Update the record location when there is a tie to keep the newer record.
+ if (newComparisonValue.compareTo(currentRecordLocation.getComparisonValue()) >= 0) {
+ int currentDocId = currentRecordLocation.getDocId();
+ if (segment == currentSegment) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ currentRecordLocation.getDistinctSegmentCount());
+ } else {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentRecordLocation.getDistinctSegmentCount()));
+ }
+ } else {
+ // Out-of-order record
+ handleOutOfOrderEvent(currentRecordLocation.getComparisonValue(), recordInfo.getComparisonValue());
+ isOutOfOrderRecord.set(true);
+ if (segment == currentSegment) {
+ return currentRecordLocation;
+ } else {
+ return new RecordLocation(currentSegment, currentRecordLocation.getDocId(),
+ currentRecordLocation.getComparisonValue(),
+ _context.isDropOutOfOrderRecord() ? currentRecordLocation.getDistinctSegmentCount()
+ : RecordLocation.incrementSegmentCount(currentRecordLocation.getDistinctSegmentCount()));
+ }
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+
+ updatePrimaryKeyGauge();
+ return !isOutOfOrderRecord.get();
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, MutableRoaringBitmap validDocIds) {
+ }
+
+ @Override
+ protected GenericRow doUpdateRecord(GenericRow record, RecordInfo recordInfo) {
+ assert _partialUpsertHandler != null;
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (pk, recordLocation) -> {
+ // Read the previous record if the following conditions are met:
+ // - New record is not a DELETE record
+ // - New record is not out-of-order
+ // - Previous record is not deleted
+ 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)) {
+ _reusePreviousRow.init(currentSegment, currentDocId);
+ _partialUpsertHandler.merge(_reusePreviousRow, record, _reuseMergeResultHolder);
+ _reuseMergeResultHolder.clear();
+ }
+ }
+ return recordLocation;
+ });
+ return record;
+ }
+
+ @VisibleForTesting
+ static class RecordLocation {
+ private final IndexSegment _segment;
+ private final int _docId;
+ private final Comparable _comparisonValue;
+ // The number of distinct segments in which the record is present. If this count is less than or equal to 1,
+ // we proceed to remove the record from the primary hashmap during the deletedKeysTTL process.
+ private final int _distinctSegmentCount;
+
+ public RecordLocation(IndexSegment indexSegment, int docId, Comparable comparisonValue, int distinctSegmentCount) {
+ _segment = indexSegment;
+ _docId = docId;
+ _comparisonValue = comparisonValue;
+ _distinctSegmentCount = distinctSegmentCount;
+ }
+
+ public static int incrementSegmentCount(int count) {
+ return count + 1;
+ }
+
+ public static int decrementSegmentCount(int count) {
+ return count - 1;
+ }
Review Comment:
(minor) I don't think these 2 helper method is necessary. I found them less readable them simply `+1` or `-1`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,391 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'enableConsistentDeletes' set to true.
+ *
+ * Consistent deletion ensures that when deletedKeysTTL is enabled with UpsertCompaction, the key metadata is
+ * removed from the HashMap only after all other records in the old segments are compacted. This guarantees
+ * data consistency. Without this, there can be a scenario where a deleted record is compacted first, while an
+ * old record remains non-compacted in a previous segment. During a server restart, this could lead to the old
+ * record reappearing. For the end-user, this would result in a data loss or inconsistency scenario, as the
+ * record was marked for deletion.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ throw new UnsupportedOperationException("Consistent-deletion does not support preloading of segments.");
+ }
+
+ @Override
+ protected void doRemoveSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing {} segment: {}, current primary key count: {}",
+ segment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName, getNumPrimaryKeys());
+ long startTimeMs = System.currentTimeMillis();
+
+ try (
+ UpsertUtils.PrimaryKeyReader primaryKeyReader = new UpsertUtils.PrimaryKeyReader(segment, _primaryKeyColumns)) {
+ removeSegment(segment,
+ UpsertUtils.getPrimaryKeyIterator(primaryKeyReader, segment.getSegmentMetadata().getTotalDocs()));
+ } catch (Exception e) {
+ throw new RuntimeException(
+ String.format("Caught exception while removing segment: %s, table: %s", segment.getSegmentName(),
+ _tableNameWithType), e);
+ }
+
+ // Update metrics
+ long numPrimaryKeys = getNumPrimaryKeys();
+ updatePrimaryKeyGauge(numPrimaryKeys);
+ _logger.info("Finished removing segment: {} in {}ms, current primary key count: {}", segmentName,
+ System.currentTimeMillis() - startTimeMs, numPrimaryKeys);
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, Iterator<PrimaryKey> primaryKeyIterator) {
+ Set<Object> deDuplicatedPrimaryKeys = new HashSet<>();
+ while (primaryKeyIterator.hasNext()) {
+ PrimaryKey primaryKey = primaryKeyIterator.next();
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(primaryKey, _hashFunction),
+ (pk, recordLocation) -> {
+ if (deDuplicatedPrimaryKeys.contains(pk)) {
+ return recordLocation;
+ }
+ deDuplicatedPrimaryKeys.add(pk);
Review Comment:
(minor)
```suggestion
if (!uniquePrimaryKeys.add(pk)) {
return recordLocation;
}
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/TableUpsertMetadataManagerFactory.java:
##########
@@ -83,8 +82,14 @@ public static TableUpsertMetadataManager create(TableConfig tableConfig,
metadataManagerClass, tableNameWithType), e);
}
} else {
- LOGGER.info("Creating ConcurrentMapTableUpsertMetadataManager for table: {}", tableNameWithType);
- metadataManager = new ConcurrentMapTableUpsertMetadataManager();
+ if (upsertConfig.isEnableConsistentDeletes()) {
Review Comment:
Since the new `ConcurrentMapTableUpsertMetadataManagerForConsistentDeletes` is almost the same as `ConcurrentMapTableUpsertMetadataManager`, I guess we can move the logic into `ConcurrentMapTableUpsertMetadataManager.getOrCreatePartitionManager()` and simply change the value type of `_partitionMetadataManagerMap` to `BasePartitionUpsertMetadataManager`
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/UpsertConfig.java:
##########
@@ -79,6 +79,9 @@ public enum ConsistencyMode {
@JsonPropertyDescription("Whether to preload segments for fast upsert metadata recovery")
private boolean _enablePreload;
+ @JsonPropertyDescription("If we are using deletion + compaction we need to enable this for data consistency")
Review Comment:
This only associates with `_deletedKeysTTL`, so suggest moving it next to `_deletedKeysTTL` and make the name and description more specific. Currently people might misunderstand this to be associated with `_deleteRecordColumn`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,391 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'enableConsistentDeletes' set to true.
+ *
+ * Consistent deletion ensures that when deletedKeysTTL is enabled with UpsertCompaction, the key metadata is
+ * removed from the HashMap only after all other records in the old segments are compacted. This guarantees
+ * data consistency. Without this, there can be a scenario where a deleted record is compacted first, while an
+ * old record remains non-compacted in a previous segment. During a server restart, this could lead to the old
+ * record reappearing. For the end-user, this would result in a data loss or inconsistency scenario, as the
+ * record was marked for deletion.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ throw new UnsupportedOperationException("Consistent-deletion does not support preloading of segments.");
+ }
+
+ @Override
+ protected void doRemoveSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing {} segment: {}, current primary key count: {}",
+ segment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName, getNumPrimaryKeys());
+ long startTimeMs = System.currentTimeMillis();
+
+ try (
+ UpsertUtils.PrimaryKeyReader primaryKeyReader = new UpsertUtils.PrimaryKeyReader(segment, _primaryKeyColumns)) {
+ removeSegment(segment,
+ UpsertUtils.getPrimaryKeyIterator(primaryKeyReader, segment.getSegmentMetadata().getTotalDocs()));
+ } catch (Exception e) {
+ throw new RuntimeException(
+ String.format("Caught exception while removing segment: %s, table: %s", segment.getSegmentName(),
+ _tableNameWithType), e);
+ }
+
+ // Update metrics
+ long numPrimaryKeys = getNumPrimaryKeys();
+ updatePrimaryKeyGauge(numPrimaryKeys);
+ _logger.info("Finished removing segment: {} in {}ms, current primary key count: {}", segmentName,
+ System.currentTimeMillis() - startTimeMs, numPrimaryKeys);
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, Iterator<PrimaryKey> primaryKeyIterator) {
+ Set<Object> deDuplicatedPrimaryKeys = new HashSet<>();
Review Comment:
(minor)
```suggestion
Set<Object> uniquePrimaryKeys = new HashSet<>();
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,391 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'enableConsistentDeletes' set to true.
+ *
+ * Consistent deletion ensures that when deletedKeysTTL is enabled with UpsertCompaction, the key metadata is
+ * removed from the HashMap only after all other records in the old segments are compacted. This guarantees
+ * data consistency. Without this, there can be a scenario where a deleted record is compacted first, while an
+ * old record remains non-compacted in a previous segment. During a server restart, this could lead to the old
+ * record reappearing. For the end-user, this would result in a data loss or inconsistency scenario, as the
+ * record was marked for deletion.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ throw new UnsupportedOperationException("Consistent-deletion does not support preloading of segments.");
+ }
+
+ @Override
+ protected void doRemoveSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing {} segment: {}, current primary key count: {}",
+ segment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName, getNumPrimaryKeys());
+ long startTimeMs = System.currentTimeMillis();
+
+ try (
+ UpsertUtils.PrimaryKeyReader primaryKeyReader = new UpsertUtils.PrimaryKeyReader(segment, _primaryKeyColumns)) {
+ removeSegment(segment,
+ UpsertUtils.getPrimaryKeyIterator(primaryKeyReader, segment.getSegmentMetadata().getTotalDocs()));
+ } catch (Exception e) {
+ throw new RuntimeException(
+ String.format("Caught exception while removing segment: %s, table: %s", segment.getSegmentName(),
+ _tableNameWithType), e);
+ }
+
+ // Update metrics
+ long numPrimaryKeys = getNumPrimaryKeys();
+ updatePrimaryKeyGauge(numPrimaryKeys);
+ _logger.info("Finished removing segment: {} in {}ms, current primary key count: {}", segmentName,
+ System.currentTimeMillis() - startTimeMs, numPrimaryKeys);
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, Iterator<PrimaryKey> primaryKeyIterator) {
+ Set<Object> deDuplicatedPrimaryKeys = new HashSet<>();
Review Comment:
Please also add some comments explaining why we need to dedup it
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1714392241
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/UpsertConfig.java:
##########
@@ -79,6 +79,9 @@ public enum ConsistencyMode {
@JsonPropertyDescription("Whether to preload segments for fast upsert metadata recovery")
private boolean _enablePreload;
+ @JsonPropertyDescription("If we are using deletion + compaction we need to enable this for data consistency")
Review Comment:
Yes i renamed it to `_enableDeletedKeysCompactionConsistency`. It's updated in UpsertConfig class. Pushing a change to update in `UpsertContext` as well.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1714403027
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,391 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'enableConsistentDeletes' set to true.
+ *
+ * Consistent deletion ensures that when deletedKeysTTL is enabled with UpsertCompaction, the key metadata is
+ * removed from the HashMap only after all other records in the old segments are compacted. This guarantees
+ * data consistency. Without this, there can be a scenario where a deleted record is compacted first, while an
+ * old record remains non-compacted in a previous segment. During a server restart, this could lead to the old
+ * record reappearing. For the end-user, this would result in a data loss or inconsistency scenario, as the
+ * record was marked for deletion.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ throw new UnsupportedOperationException("Consistent-deletion does not support preloading of segments.");
+ }
+
+ @Override
+ protected void doRemoveSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing {} segment: {}, current primary key count: {}",
+ segment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName, getNumPrimaryKeys());
+ long startTimeMs = System.currentTimeMillis();
+
+ try (
+ UpsertUtils.PrimaryKeyReader primaryKeyReader = new UpsertUtils.PrimaryKeyReader(segment, _primaryKeyColumns)) {
+ removeSegment(segment,
+ UpsertUtils.getPrimaryKeyIterator(primaryKeyReader, segment.getSegmentMetadata().getTotalDocs()));
+ } catch (Exception e) {
+ throw new RuntimeException(
+ String.format("Caught exception while removing segment: %s, table: %s", segment.getSegmentName(),
+ _tableNameWithType), e);
+ }
+
+ // Update metrics
+ long numPrimaryKeys = getNumPrimaryKeys();
+ updatePrimaryKeyGauge(numPrimaryKeys);
+ _logger.info("Finished removing segment: {} in {}ms, current primary key count: {}", segmentName,
+ System.currentTimeMillis() - startTimeMs, numPrimaryKeys);
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, Iterator<PrimaryKey> primaryKeyIterator) {
+ Set<Object> deDuplicatedPrimaryKeys = new HashSet<>();
+ while (primaryKeyIterator.hasNext()) {
+ PrimaryKey primaryKey = primaryKeyIterator.next();
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(primaryKey, _hashFunction),
+ (pk, recordLocation) -> {
+ if (deDuplicatedPrimaryKeys.contains(pk)) {
+ return recordLocation;
+ }
+ deDuplicatedPrimaryKeys.add(pk);
+ if (recordLocation.getSegment() == segment) {
+ return null;
+ }
+ return new RecordLocation(recordLocation.getSegment(), recordLocation.getDocId(),
+ recordLocation.getComparisonValue(),
+ RecordLocation.decrementSegmentCount(recordLocation.getDistinctSegmentCount()));
+ });
+ }
+ }
+
+ @Override
+ public void doRemoveExpiredPrimaryKeys() {
+ AtomicInteger numDeletedTTLKeysRemoved = new AtomicInteger();
+ double largestSeenComparisonValue = _largestSeenComparisonValue.get();
+ double deletedKeysThreshold;
+ if (_deletedKeysTTL > 0) {
+ deletedKeysThreshold = largestSeenComparisonValue - _deletedKeysTTL;
+ } else {
+ deletedKeysThreshold = Double.MIN_VALUE;
+ }
+
+ _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
+ double comparisonValue = ((Number) recordLocation.getComparisonValue()).doubleValue();
+ // We need to verify that the record belongs to only one segment. If a record is part of multiple segments,
+ // an issue can arise where the upsert compaction might first process the segment containing the delete record
+ // while the previous segment(s) are not compacted. Upon restart, this can inadvertently revive the key
+ // that was originally marked for deletion.
+ if (_deletedKeysTTL > 0 && comparisonValue < deletedKeysThreshold
+ && recordLocation.getDistinctSegmentCount() <= 1) {
+ ThreadSafeMutableRoaringBitmap currentQueryableDocIds = recordLocation.getSegment().getQueryableDocIds();
+ // if key not part of queryable doc id, it means it is deleted
+ if (currentQueryableDocIds != null && !currentQueryableDocIds.contains(recordLocation.getDocId())) {
+ _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ removeDocId(recordLocation.getSegment(), recordLocation.getDocId());
+ numDeletedTTLKeysRemoved.getAndIncrement();
+ }
+ }
+ });
+
+ // Update metrics
+ updatePrimaryKeyGauge();
+ int numDeletedTTLKeys = numDeletedTTLKeysRemoved.get();
+ if (numDeletedTTLKeys > 0) {
+ _logger.info("Deleted {} primary keys based on deletedKeysTTL", numDeletedTTLKeys);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.DELETED_KEYS_TTL_PRIMARY_KEYS_REMOVED,
+ numDeletedTTLKeys);
+ }
+ }
+
+ @Override
+ protected boolean doAddRecord(MutableSegment segment, RecordInfo recordInfo) {
+ AtomicBoolean isOutOfOrderRecord = new AtomicBoolean(false);
+ ThreadSafeMutableRoaringBitmap validDocIds = Objects.requireNonNull(segment.getValidDocIds());
+ ThreadSafeMutableRoaringBitmap queryableDocIds = segment.getQueryableDocIds();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+
+ // When TTL is enabled, update largestSeenComparisonValue when adding new record
+ if (_deletedKeysTTL > 0) {
+ double comparisonValue = ((Number) newComparisonValue).doubleValue();
+ _largestSeenComparisonValue.getAndUpdate(v -> Math.max(v, comparisonValue));
+ }
+
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ // Update the record location when the new comparison value is greater than or equal to the current value.
+ // Update the record location when there is a tie to keep the newer record.
+ if (newComparisonValue.compareTo(currentRecordLocation.getComparisonValue()) >= 0) {
+ int currentDocId = currentRecordLocation.getDocId();
+ if (segment == currentSegment) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ currentRecordLocation.getDistinctSegmentCount());
+ } else {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentRecordLocation.getDistinctSegmentCount()));
+ }
+ } else {
+ // Out-of-order record
+ handleOutOfOrderEvent(currentRecordLocation.getComparisonValue(), recordInfo.getComparisonValue());
+ isOutOfOrderRecord.set(true);
+ if (segment == currentSegment) {
+ return currentRecordLocation;
+ } else {
+ return new RecordLocation(currentSegment, currentRecordLocation.getDocId(),
+ currentRecordLocation.getComparisonValue(),
+ _context.isDropOutOfOrderRecord() ? currentRecordLocation.getDistinctSegmentCount()
+ : RecordLocation.incrementSegmentCount(currentRecordLocation.getDistinctSegmentCount()));
+ }
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+
+ updatePrimaryKeyGauge();
+ return !isOutOfOrderRecord.get();
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, MutableRoaringBitmap validDocIds) {
+ }
+
+ @Override
+ protected GenericRow doUpdateRecord(GenericRow record, RecordInfo recordInfo) {
+ assert _partialUpsertHandler != null;
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (pk, recordLocation) -> {
+ // Read the previous record if the following conditions are met:
+ // - New record is not a DELETE record
+ // - New record is not out-of-order
+ // - Previous record is not deleted
+ 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)) {
+ _reusePreviousRow.init(currentSegment, currentDocId);
+ _partialUpsertHandler.merge(_reusePreviousRow, record, _reuseMergeResultHolder);
+ _reuseMergeResultHolder.clear();
+ }
+ }
+ return recordLocation;
+ });
+ return record;
+ }
+
+ @VisibleForTesting
+ static class RecordLocation {
+ private final IndexSegment _segment;
+ private final int _docId;
+ private final Comparable _comparisonValue;
+ // The number of distinct segments in which the record is present. If this count is less than or equal to 1,
+ // we proceed to remove the record from the primary hashmap during the deletedKeysTTL process.
+ private final int _distinctSegmentCount;
+
+ public RecordLocation(IndexSegment indexSegment, int docId, Comparable comparisonValue, int distinctSegmentCount) {
+ _segment = indexSegment;
+ _docId = docId;
+ _comparisonValue = comparisonValue;
+ _distinctSegmentCount = distinctSegmentCount;
+ }
+
+ public static int incrementSegmentCount(int count) {
+ return count + 1;
+ }
+
+ public static int decrementSegmentCount(int count) {
+ return count - 1;
+ }
Review Comment:
can we do it in a follow-up? initially this will help me in debugging if we are missing out on any edge cases while doing addition / subtraction? 😅
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/UpsertConfig.java:
##########
@@ -79,6 +79,9 @@ public enum ConsistencyMode {
@JsonPropertyDescription("Whether to preload segments for fast upsert metadata recovery")
private boolean _enablePreload;
+ @JsonPropertyDescription("If we are using deletion + compaction we need to enable this for data consistency")
Review Comment:
Updated!
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1683107875
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,373 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'consistentDeletes' set to true.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
Review Comment:
> What's the main reason for a new metadata manager? Is it because we are adding a new int to RecordLocation, and there are perf concerns?
Yes the main reason is using a new manager class is the int field addition to RecordLocation which we discussed will add 16bytes per key(unsigned) and for supporting 100M per host can easily add 1600MB. We thought of making RecordLocation an interface as well to achieve this but then the metadataManager class will become a bit incomprehensible with lots of if-else's.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1700864392
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/TableUpsertMetadataManagerFactory.java:
##########
@@ -29,16 +29,15 @@
public class TableUpsertMetadataManagerFactory {
- private TableUpsertMetadataManagerFactory() {
- }
-
- private static final Logger LOGGER = LoggerFactory.getLogger(TableUpsertMetadataManagerFactory.class);
public static final String UPSERT_DEFAULT_METADATA_MANAGER_CLASS = "default.metadata.manager.class";
public static final String UPSERT_DEFAULT_ENABLE_SNAPSHOT = "default.enable.snapshot";
public static final String UPSERT_DEFAULT_ENABLE_PRELOAD = "default.enable.preload";
-
public static final String UPSERT_DEFAULT_ALLOW_PARTIAL_UPSERT_CONSUMPTION_DURING_COMMIT =
"default.allow.partial.upsert.consumption.during.commit";
+ private static final Logger LOGGER = LoggerFactory.getLogger(TableUpsertMetadataManagerFactory.class);
Review Comment:
no need to touch this class if we use metadataManagerClass to enable the new feature
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,373 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'consistentDeletes' set to true.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
Review Comment:
perhaps add more comments to explain what's `consistent deletions`, which is to make compaction task works correctly with deletedKeysTTL, so that compaction removes the deletion tombstone records only after all the previous records of the same PK are removed
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BaseTableUpsertMetadataManager.java:
##########
@@ -81,6 +81,8 @@ public void init(TableConfig tableConfig, Schema schema, TableDataManager tableD
.setPartialUpsertHandler(partialUpsertHandler).setEnableSnapshot(enableSnapshot).setEnablePreload(enablePreload)
.setMetadataTTL(metadataTTL).setDeletedKeysTTL(deletedKeysTTL).setConsistencyMode(_consistencyMode)
.setUpsertViewRefreshIntervalMs(upsertViewRefreshIntervalMs).setTableIndexDir(tableIndexDir)
+ .setDropOutOfOrderRecord(upsertConfig.isDropOutOfOrderRecord())
+ .setEnableConsistentDeletes(upsertConfig.isEnableConsistentDeletes())
Review Comment:
this upsert config may be used as the feature flag: `metadataManagerClass` , so we don't need to add a new one
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,373 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'consistentDeletes' set to true.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ currentDistinctSegmentCount + 1);
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ currentDistinctSegmentCount + 1);
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
Review Comment:
throw unsupported exception as you explained in the PR description?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,373 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'consistentDeletes' set to true.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ currentDistinctSegmentCount + 1);
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ currentDistinctSegmentCount + 1);
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ _primaryKeyToRecordLocationMap.put(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ new RecordLocation(segment, newDocId, newComparisonValue, 1));
+ }
+ }
+
+ @Override
+ protected void doRemoveSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing {} segment: {}, current primary key count: {}",
+ segment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName, getNumPrimaryKeys());
+ long startTimeMs = System.currentTimeMillis();
+
+ try (
+ UpsertUtils.PrimaryKeyReader primaryKeyReader = new UpsertUtils.PrimaryKeyReader(segment, _primaryKeyColumns)) {
+ removeSegment(segment,
+ UpsertUtils.getPrimaryKeyIterator(primaryKeyReader, segment.getSegmentMetadata().getTotalDocs()));
+ } catch (Exception e) {
+ throw new RuntimeException(
+ String.format("Caught exception while removing segment: %s, table: %s", segment.getSegmentName(),
+ _tableNameWithType), e);
+ }
+
+ // Update metrics
+ long numPrimaryKeys = getNumPrimaryKeys();
+ updatePrimaryKeyGauge(numPrimaryKeys);
+ _logger.info("Finished removing segment: {} in {}ms, current primary key count: {}", segmentName,
+ System.currentTimeMillis() - startTimeMs, numPrimaryKeys);
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, Iterator<PrimaryKey> primaryKeyIterator) {
+ Set<Object> deDuplicatedPrimaryKeys = new HashSet<>();
+ while (primaryKeyIterator.hasNext()) {
+ PrimaryKey primaryKey = primaryKeyIterator.next();
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(primaryKey, _hashFunction),
+ (pk, recordLocation) -> {
+ if (deDuplicatedPrimaryKeys.contains(pk)) {
+ return recordLocation;
+ }
+ deDuplicatedPrimaryKeys.add(pk);
+ if (recordLocation.getSegment() == segment) {
+ return null;
+ }
+ return new RecordLocation(recordLocation.getSegment(), recordLocation.getDocId(),
+ recordLocation.getComparisonValue(), recordLocation.getDistinctSegmentCount() - 1);
Review Comment:
add util method to +1 and -1 inside RecordLocation class to make it easier to check where they are called.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,373 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'consistentDeletes' set to true.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ currentDistinctSegmentCount + 1);
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ currentDistinctSegmentCount + 1);
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ _primaryKeyToRecordLocationMap.put(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ new RecordLocation(segment, newDocId, newComparisonValue, 1));
+ }
+ }
+
+ @Override
+ protected void doRemoveSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing {} segment: {}, current primary key count: {}",
+ segment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName, getNumPrimaryKeys());
+ long startTimeMs = System.currentTimeMillis();
+
+ try (
+ UpsertUtils.PrimaryKeyReader primaryKeyReader = new UpsertUtils.PrimaryKeyReader(segment, _primaryKeyColumns)) {
+ removeSegment(segment,
+ UpsertUtils.getPrimaryKeyIterator(primaryKeyReader, segment.getSegmentMetadata().getTotalDocs()));
+ } catch (Exception e) {
+ throw new RuntimeException(
+ String.format("Caught exception while removing segment: %s, table: %s", segment.getSegmentName(),
+ _tableNameWithType), e);
+ }
+
+ // Update metrics
+ long numPrimaryKeys = getNumPrimaryKeys();
+ updatePrimaryKeyGauge(numPrimaryKeys);
+ _logger.info("Finished removing segment: {} in {}ms, current primary key count: {}", segmentName,
+ System.currentTimeMillis() - startTimeMs, numPrimaryKeys);
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, Iterator<PrimaryKey> primaryKeyIterator) {
+ Set<Object> deDuplicatedPrimaryKeys = new HashSet<>();
+ while (primaryKeyIterator.hasNext()) {
+ PrimaryKey primaryKey = primaryKeyIterator.next();
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(primaryKey, _hashFunction),
+ (pk, recordLocation) -> {
+ if (deDuplicatedPrimaryKeys.contains(pk)) {
+ return recordLocation;
+ }
+ deDuplicatedPrimaryKeys.add(pk);
+ if (recordLocation.getSegment() == segment) {
+ return null;
+ }
+ return new RecordLocation(recordLocation.getSegment(), recordLocation.getDocId(),
+ recordLocation.getComparisonValue(), recordLocation.getDistinctSegmentCount() - 1);
+ });
+ }
+ }
+
+ @Override
+ public void doRemoveExpiredPrimaryKeys() {
+ AtomicInteger numDeletedTTLKeysRemoved = new AtomicInteger();
+ double largestSeenComparisonValue = _largestSeenComparisonValue.get();
+ double deletedKeysThreshold;
+ if (_deletedKeysTTL > 0) {
+ deletedKeysThreshold = largestSeenComparisonValue - _deletedKeysTTL;
+ } else {
+ deletedKeysThreshold = Double.MIN_VALUE;
+ }
+
+ _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
+ double comparisonValue = ((Number) recordLocation.getComparisonValue()).doubleValue();
+ if (_deletedKeysTTL > 0 && comparisonValue < deletedKeysThreshold
+ && recordLocation.getDistinctSegmentCount() <= 1) {
+ ThreadSafeMutableRoaringBitmap currentQueryableDocIds = recordLocation.getSegment().getQueryableDocIds();
+ // if key not part of queryable doc id, it means it is deleted
+ if (currentQueryableDocIds != null && !currentQueryableDocIds.contains(recordLocation.getDocId())) {
+ _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ removeDocId(recordLocation.getSegment(), recordLocation.getDocId());
+ numDeletedTTLKeysRemoved.getAndIncrement();
+ }
+ }
+ });
+
+ // Update metrics
+ updatePrimaryKeyGauge();
+ int numDeletedTTLKeys = numDeletedTTLKeysRemoved.get();
+ if (numDeletedTTLKeys > 0) {
+ _logger.info("Deleted {} primary keys based on deletedKeysTTL", numDeletedTTLKeys);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.DELETED_KEYS_TTL_PRIMARY_KEYS_REMOVED,
+ numDeletedTTLKeys);
+ }
+ }
+
+ @Override
+ protected boolean doAddRecord(MutableSegment segment, RecordInfo recordInfo) {
+ AtomicBoolean isOutOfOrderRecord = new AtomicBoolean(false);
+ ThreadSafeMutableRoaringBitmap validDocIds = Objects.requireNonNull(segment.getValidDocIds());
+ ThreadSafeMutableRoaringBitmap queryableDocIds = segment.getQueryableDocIds();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+
+ // When TTL is enabled, update largestSeenComparisonValue when adding new record
+ if (_deletedKeysTTL > 0) {
+ double comparisonValue = ((Number) newComparisonValue).doubleValue();
+ _largestSeenComparisonValue.getAndUpdate(v -> Math.max(v, comparisonValue));
+ }
+
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ // Update the record location when the new comparison value is greater than or equal to the current value.
+ // Update the record location when there is a tie to keep the newer record.
+ if (newComparisonValue.compareTo(currentRecordLocation.getComparisonValue()) >= 0) {
+ int currentDocId = currentRecordLocation.getDocId();
+ if (segment == currentSegment) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ currentRecordLocation.getDistinctSegmentCount());
+ } else {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ currentRecordLocation.getDistinctSegmentCount() + 1);
+ }
+ } else {
+ // Out-of-order record
+ handleOutOfOrderEvent(currentRecordLocation.getComparisonValue(), recordInfo.getComparisonValue());
+ isOutOfOrderRecord.set(true);
+ if (segment == currentSegment) {
+ return currentRecordLocation;
+ } else {
+ return new RecordLocation(currentSegment, currentRecordLocation.getDocId(),
+ currentRecordLocation.getComparisonValue(),
+ currentRecordLocation.getDistinctSegmentCount() + (_context.isDropOutOfOrderRecord() ? 0 : 1));
+ }
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+
+ updatePrimaryKeyGauge();
+ return !isOutOfOrderRecord.get();
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, MutableRoaringBitmap validDocIds) {
+ }
+
+ @Override
+ protected GenericRow doUpdateRecord(GenericRow record, RecordInfo recordInfo) {
+ assert _partialUpsertHandler != null;
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (pk, recordLocation) -> {
+ // Read the previous record if the following conditions are met:
+ // - New record is not a DELETE record
+ // - New record is not out-of-order
+ // - Previous record is not deleted
+ 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)) {
+ _reusePreviousRow.init(currentSegment, currentDocId);
+ _partialUpsertHandler.merge(_reusePreviousRow, record, _reuseMergeResultHolder);
+ _reuseMergeResultHolder.clear();
+ }
+ }
+ return recordLocation;
+ });
+ return record;
+ }
+
+ @VisibleForTesting
+ static class RecordLocation {
+ private final IndexSegment _segment;
+ private final int _docId;
+ private final Comparable _comparisonValue;
+ private final int _distinctSegmentCount;
+
+ public RecordLocation(IndexSegment indexSegment, int docId, Comparable comparisonValue, int distinctSegmentCount) {
+ _segment = indexSegment;
+ _docId = docId;
+ _comparisonValue = comparisonValue;
+ _distinctSegmentCount = distinctSegmentCount;
+ }
+
+ public IndexSegment getSegment() {
+ return _segment;
+ }
+
+ public int getDocId() {
+ return _docId;
+ }
+
+ public Comparable getComparisonValue() {
+ return _comparisonValue;
+ }
+
+ public int getDistinctSegmentCount() {
Review Comment:
where do you want to check if this value is > 1 and let compaction tasks skip the deletedKeys?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1712723908
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/UpsertConfig.java:
##########
@@ -79,6 +79,9 @@ public enum ConsistencyMode {
@JsonPropertyDescription("Whether to preload segments for fast upsert metadata recovery")
private boolean _enablePreload;
+ @JsonPropertyDescription("If we are using deletion + compaction we need to enable this for data consistency")
Review Comment:
Updated! Let me know if we can go for a better naming.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1714385181
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,391 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'enableConsistentDeletes' set to true.
+ *
+ * Consistent deletion ensures that when deletedKeysTTL is enabled with UpsertCompaction, the key metadata is
+ * removed from the HashMap only after all other records in the old segments are compacted. This guarantees
+ * data consistency. Without this, there can be a scenario where a deleted record is compacted first, while an
+ * old record remains non-compacted in a previous segment. During a server restart, this could lead to the old
+ * record reappearing. For the end-user, this would result in a data loss or inconsistency scenario, as the
+ * record was marked for deletion.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ RecordLocation.incrementSegmentCount(currentDistinctSegmentCount));
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ throw new UnsupportedOperationException("Consistent-deletion does not support preloading of segments.");
+ }
+
+ @Override
+ protected void doRemoveSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing {} segment: {}, current primary key count: {}",
+ segment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName, getNumPrimaryKeys());
+ long startTimeMs = System.currentTimeMillis();
+
+ try (
+ UpsertUtils.PrimaryKeyReader primaryKeyReader = new UpsertUtils.PrimaryKeyReader(segment, _primaryKeyColumns)) {
+ removeSegment(segment,
+ UpsertUtils.getPrimaryKeyIterator(primaryKeyReader, segment.getSegmentMetadata().getTotalDocs()));
+ } catch (Exception e) {
+ throw new RuntimeException(
+ String.format("Caught exception while removing segment: %s, table: %s", segment.getSegmentName(),
+ _tableNameWithType), e);
+ }
+
+ // Update metrics
+ long numPrimaryKeys = getNumPrimaryKeys();
+ updatePrimaryKeyGauge(numPrimaryKeys);
+ _logger.info("Finished removing segment: {} in {}ms, current primary key count: {}", segmentName,
+ System.currentTimeMillis() - startTimeMs, numPrimaryKeys);
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, Iterator<PrimaryKey> primaryKeyIterator) {
+ Set<Object> deDuplicatedPrimaryKeys = new HashSet<>();
+ while (primaryKeyIterator.hasNext()) {
+ PrimaryKey primaryKey = primaryKeyIterator.next();
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(primaryKey, _hashFunction),
+ (pk, recordLocation) -> {
+ if (deDuplicatedPrimaryKeys.contains(pk)) {
+ return recordLocation;
+ }
+ deDuplicatedPrimaryKeys.add(pk);
+ if (recordLocation.getSegment() == segment) {
+ return null;
+ }
+ return new RecordLocation(recordLocation.getSegment(), recordLocation.getDocId(),
+ recordLocation.getComparisonValue(),
+ RecordLocation.decrementSegmentCount(recordLocation.getDistinctSegmentCount()));
+ });
+ }
+ }
+
+ @Override
+ public void doRemoveExpiredPrimaryKeys() {
+ AtomicInteger numDeletedTTLKeysRemoved = new AtomicInteger();
+ double largestSeenComparisonValue = _largestSeenComparisonValue.get();
+ double deletedKeysThreshold;
+ if (_deletedKeysTTL > 0) {
+ deletedKeysThreshold = largestSeenComparisonValue - _deletedKeysTTL;
+ } else {
+ deletedKeysThreshold = Double.MIN_VALUE;
+ }
+
+ _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
+ double comparisonValue = ((Number) recordLocation.getComparisonValue()).doubleValue();
+ // We need to verify that the record belongs to only one segment. If a record is part of multiple segments,
+ // an issue can arise where the upsert compaction might first process the segment containing the delete record
+ // while the previous segment(s) are not compacted. Upon restart, this can inadvertently revive the key
+ // that was originally marked for deletion.
+ if (_deletedKeysTTL > 0 && comparisonValue < deletedKeysThreshold
+ && recordLocation.getDistinctSegmentCount() <= 1) {
+ ThreadSafeMutableRoaringBitmap currentQueryableDocIds = recordLocation.getSegment().getQueryableDocIds();
+ // if key not part of queryable doc id, it means it is deleted
+ if (currentQueryableDocIds != null && !currentQueryableDocIds.contains(recordLocation.getDocId())) {
+ _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ removeDocId(recordLocation.getSegment(), recordLocation.getDocId());
+ numDeletedTTLKeysRemoved.getAndIncrement();
+ }
+ }
+ });
+
+ // Update metrics
+ updatePrimaryKeyGauge();
+ int numDeletedTTLKeys = numDeletedTTLKeysRemoved.get();
+ if (numDeletedTTLKeys > 0) {
+ _logger.info("Deleted {} primary keys based on deletedKeysTTL", numDeletedTTLKeys);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.DELETED_KEYS_TTL_PRIMARY_KEYS_REMOVED,
+ numDeletedTTLKeys);
+ }
+ }
+
+ @Override
+ protected boolean doAddRecord(MutableSegment segment, RecordInfo recordInfo) {
+ AtomicBoolean isOutOfOrderRecord = new AtomicBoolean(false);
+ ThreadSafeMutableRoaringBitmap validDocIds = Objects.requireNonNull(segment.getValidDocIds());
+ ThreadSafeMutableRoaringBitmap queryableDocIds = segment.getQueryableDocIds();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+
+ // When TTL is enabled, update largestSeenComparisonValue when adding new record
+ if (_deletedKeysTTL > 0) {
+ double comparisonValue = ((Number) newComparisonValue).doubleValue();
+ _largestSeenComparisonValue.getAndUpdate(v -> Math.max(v, comparisonValue));
+ }
+
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ // Update the record location when the new comparison value is greater than or equal to the current value.
+ // Update the record location when there is a tie to keep the newer record.
+ if (newComparisonValue.compareTo(currentRecordLocation.getComparisonValue()) >= 0) {
+ int currentDocId = currentRecordLocation.getDocId();
+ if (segment == currentSegment) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ currentRecordLocation.getDistinctSegmentCount());
+ } else {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ RecordLocation.incrementSegmentCount(currentRecordLocation.getDistinctSegmentCount()));
+ }
+ } else {
+ // Out-of-order record
+ handleOutOfOrderEvent(currentRecordLocation.getComparisonValue(), recordInfo.getComparisonValue());
+ isOutOfOrderRecord.set(true);
+ if (segment == currentSegment) {
+ return currentRecordLocation;
+ } else {
+ return new RecordLocation(currentSegment, currentRecordLocation.getDocId(),
+ currentRecordLocation.getComparisonValue(),
+ _context.isDropOutOfOrderRecord() ? currentRecordLocation.getDistinctSegmentCount()
+ : RecordLocation.incrementSegmentCount(currentRecordLocation.getDistinctSegmentCount()));
+ }
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+
+ updatePrimaryKeyGauge();
+ return !isOutOfOrderRecord.get();
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, MutableRoaringBitmap validDocIds) {
+ }
+
+ @Override
+ protected GenericRow doUpdateRecord(GenericRow record, RecordInfo recordInfo) {
+ assert _partialUpsertHandler != null;
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (pk, recordLocation) -> {
+ // Read the previous record if the following conditions are met:
+ // - New record is not a DELETE record
+ // - New record is not out-of-order
+ // - Previous record is not deleted
+ 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)) {
+ _reusePreviousRow.init(currentSegment, currentDocId);
+ _partialUpsertHandler.merge(_reusePreviousRow, record, _reuseMergeResultHolder);
+ _reuseMergeResultHolder.clear();
+ }
+ }
+ return recordLocation;
+ });
+ return record;
+ }
+
+ @VisibleForTesting
+ static class RecordLocation {
+ private final IndexSegment _segment;
+ private final int _docId;
+ private final Comparable _comparisonValue;
+ // The number of distinct segments in which the record is present. If this count is less than or equal to 1,
+ // we proceed to remove the record from the primary hashmap during the deletedKeysTTL process.
+ private final int _distinctSegmentCount;
+
+ public RecordLocation(IndexSegment indexSegment, int docId, Comparable comparisonValue, int distinctSegmentCount) {
+ _segment = indexSegment;
+ _docId = docId;
+ _comparisonValue = comparisonValue;
+ _distinctSegmentCount = distinctSegmentCount;
+ }
+
+ public static int incrementSegmentCount(int count) {
+ return count + 1;
+ }
+
+ public static int decrementSegmentCount(int count) {
+ return count - 1;
+ }
Review Comment:
No helper method, simply `+1` and `-1` inplace
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1683559146
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,373 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'consistentDeletes' set to true.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
Review Comment:
@Jackie-Jiang : just wanted to confirm, you folks are okay with creating a new metadata manager for handling deletes?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1683085725
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BaseTableUpsertMetadataManager.java:
##########
@@ -81,6 +81,8 @@ public void init(TableConfig tableConfig, Schema schema, TableDataManager tableD
.setPartialUpsertHandler(partialUpsertHandler).setEnableSnapshot(enableSnapshot).setEnablePreload(enablePreload)
.setMetadataTTL(metadataTTL).setDeletedKeysTTL(deletedKeysTTL).setConsistencyMode(_consistencyMode)
.setUpsertViewRefreshIntervalMs(upsertViewRefreshIntervalMs).setTableIndexDir(tableIndexDir)
+ .setDropOutOfOrderRecord(upsertConfig.isDropOutOfOrderRecord())
+ .setEnableConsistentDeletes(upsertConfig.isEnableConsistentDeletes())
Review Comment:
Why is there a separate config for this? Shouldn't we use the new metadata manager whenever deletions are enabled?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,373 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'consistentDeletes' set to true.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
Review Comment:
What's the main reason for a new metadata manager? Is it because we are adding a new int to RecordLocation, and there are perf concerns?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1701490207
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,373 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentImpl;
+import org.apache.pinot.segment.local.segment.readers.LazyRow;
+import org.apache.pinot.segment.local.utils.HashUtils;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.MutableSegment;
+import org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.data.readers.PrimaryKey;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/**
+ * Implementation of {@link PartitionUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'consistentDeletes' set to true.
+ */
+@SuppressWarnings({"rawtypes", "unchecked"})
+@ThreadSafe
+public class ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes
+ extends BasePartitionUpsertMetadataManager {
+
+ @VisibleForTesting
+ final ConcurrentHashMap<Object, ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation>
+ _primaryKeyToRecordLocationMap = new ConcurrentHashMap<>();
+ // Used to initialize a reference to previous row for merging in partial upsert
+ private final LazyRow _reusePreviousRow = new LazyRow();
+ private final Map<String, Object> _reuseMergeResultHolder = new HashMap<>();
+
+ public ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes(String tableNameWithType, int partitionId,
+ UpsertContext context) {
+ super(tableNameWithType, partitionId, context);
+ }
+
+ @Override
+ protected long getNumPrimaryKeys() {
+ return _primaryKeyToRecordLocationMap.size();
+ }
+
+ @Override
+ protected void doAddOrReplaceSegment(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator,
+ @Nullable IndexSegment oldSegment, @Nullable MutableRoaringBitmap validDocIdsForOldSegment) {
+ String segmentName = segment.getSegmentName();
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+
+ AtomicInteger numKeysInWrongSegment = new AtomicInteger();
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ int currentDocId = currentRecordLocation.getDocId();
+ Comparable currentComparisonValue = currentRecordLocation.getComparisonValue();
+ int comparisonResult = newComparisonValue.compareTo(currentComparisonValue);
+ int currentDistinctSegmentCount = currentRecordLocation.getDistinctSegmentCount();
+
+ // The current record is in the same segment
+ // Update the record location when there is a tie to keep the newer record. Note that the record info
+ // iterator will return records with incremental doc ids.
+ if (currentSegment == segment) {
+ if (comparisonResult >= 0) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletes.RecordLocation(segment,
+ newDocId, newComparisonValue, currentDistinctSegmentCount);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in an old segment being replaced
+ // This could happen when committing a consuming segment, or reloading a completed segment. In this
+ // case, we want to update the record location when there is a tie because the record locations should
+ // point to the new added segment instead of the old segment being replaced. Also, do not update the valid
+ // doc ids for the old segment because it has not been replaced yet. We pass in an optional valid doc ids
+ // snapshot for the old segment, which can be updated and used to track the docs not replaced yet.
+ if (currentSegment == oldSegment) {
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ if (validDocIdsForOldSegment != null) {
+ validDocIdsForOldSegment.remove(currentDocId);
+ }
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ currentDistinctSegmentCount + 1);
+ }
+ }
+
+ // This should not happen because the previously replaced segment should have all keys removed. We still
+ // handle it here, and also track the number of keys not properly replaced previously.
+ String currentSegmentName = currentSegment.getSegmentName();
+ if (currentSegmentName.equals(segmentName)) {
+ numKeysInWrongSegment.getAndIncrement();
+ if (comparisonResult >= 0) {
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return currentRecordLocation;
+ }
+ }
+
+ // The current record is in a different segment
+ // Update the record location when getting a newer comparison value, or the value is the same as the
+ // current value, but the segment has a larger sequence number (the segment is newer than the current
+ // segment).
+ if (comparisonResult > 0 || (comparisonResult == 0 && shouldReplaceOnComparisonTie(segmentName,
+ currentSegmentName, segment.getSegmentMetadata().getIndexCreationTime(),
+ currentSegment.getSegmentMetadata().getIndexCreationTime()))) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, currentDistinctSegmentCount + 1);
+ } else {
+ return new RecordLocation(currentSegment, currentDocId, currentComparisonValue,
+ currentDistinctSegmentCount + 1);
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+ }
+ int numKeys = numKeysInWrongSegment.get();
+ if (numKeys > 0) {
+ _logger.warn("Found {} primary keys in the wrong segment when adding segment: {}", numKeys, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.UPSERT_KEYS_IN_WRONG_SEGMENT, numKeys);
+ }
+ }
+
+ @Override
+ protected void addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds, Iterator<RecordInfo> recordInfoIterator) {
+ segment.enableUpsert(this, validDocIds, queryableDocIds);
+ while (recordInfoIterator.hasNext()) {
+ RecordInfo recordInfo = recordInfoIterator.next();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ _primaryKeyToRecordLocationMap.put(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ new RecordLocation(segment, newDocId, newComparisonValue, 1));
+ }
+ }
+
+ @Override
+ protected void doRemoveSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing {} segment: {}, current primary key count: {}",
+ segment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName, getNumPrimaryKeys());
+ long startTimeMs = System.currentTimeMillis();
+
+ try (
+ UpsertUtils.PrimaryKeyReader primaryKeyReader = new UpsertUtils.PrimaryKeyReader(segment, _primaryKeyColumns)) {
+ removeSegment(segment,
+ UpsertUtils.getPrimaryKeyIterator(primaryKeyReader, segment.getSegmentMetadata().getTotalDocs()));
+ } catch (Exception e) {
+ throw new RuntimeException(
+ String.format("Caught exception while removing segment: %s, table: %s", segment.getSegmentName(),
+ _tableNameWithType), e);
+ }
+
+ // Update metrics
+ long numPrimaryKeys = getNumPrimaryKeys();
+ updatePrimaryKeyGauge(numPrimaryKeys);
+ _logger.info("Finished removing segment: {} in {}ms, current primary key count: {}", segmentName,
+ System.currentTimeMillis() - startTimeMs, numPrimaryKeys);
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, Iterator<PrimaryKey> primaryKeyIterator) {
+ Set<Object> deDuplicatedPrimaryKeys = new HashSet<>();
+ while (primaryKeyIterator.hasNext()) {
+ PrimaryKey primaryKey = primaryKeyIterator.next();
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(primaryKey, _hashFunction),
+ (pk, recordLocation) -> {
+ if (deDuplicatedPrimaryKeys.contains(pk)) {
+ return recordLocation;
+ }
+ deDuplicatedPrimaryKeys.add(pk);
+ if (recordLocation.getSegment() == segment) {
+ return null;
+ }
+ return new RecordLocation(recordLocation.getSegment(), recordLocation.getDocId(),
+ recordLocation.getComparisonValue(), recordLocation.getDistinctSegmentCount() - 1);
+ });
+ }
+ }
+
+ @Override
+ public void doRemoveExpiredPrimaryKeys() {
+ AtomicInteger numDeletedTTLKeysRemoved = new AtomicInteger();
+ double largestSeenComparisonValue = _largestSeenComparisonValue.get();
+ double deletedKeysThreshold;
+ if (_deletedKeysTTL > 0) {
+ deletedKeysThreshold = largestSeenComparisonValue - _deletedKeysTTL;
+ } else {
+ deletedKeysThreshold = Double.MIN_VALUE;
+ }
+
+ _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
+ double comparisonValue = ((Number) recordLocation.getComparisonValue()).doubleValue();
+ if (_deletedKeysTTL > 0 && comparisonValue < deletedKeysThreshold
+ && recordLocation.getDistinctSegmentCount() <= 1) {
+ ThreadSafeMutableRoaringBitmap currentQueryableDocIds = recordLocation.getSegment().getQueryableDocIds();
+ // if key not part of queryable doc id, it means it is deleted
+ if (currentQueryableDocIds != null && !currentQueryableDocIds.contains(recordLocation.getDocId())) {
+ _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ removeDocId(recordLocation.getSegment(), recordLocation.getDocId());
+ numDeletedTTLKeysRemoved.getAndIncrement();
+ }
+ }
+ });
+
+ // Update metrics
+ updatePrimaryKeyGauge();
+ int numDeletedTTLKeys = numDeletedTTLKeysRemoved.get();
+ if (numDeletedTTLKeys > 0) {
+ _logger.info("Deleted {} primary keys based on deletedKeysTTL", numDeletedTTLKeys);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.DELETED_KEYS_TTL_PRIMARY_KEYS_REMOVED,
+ numDeletedTTLKeys);
+ }
+ }
+
+ @Override
+ protected boolean doAddRecord(MutableSegment segment, RecordInfo recordInfo) {
+ AtomicBoolean isOutOfOrderRecord = new AtomicBoolean(false);
+ ThreadSafeMutableRoaringBitmap validDocIds = Objects.requireNonNull(segment.getValidDocIds());
+ ThreadSafeMutableRoaringBitmap queryableDocIds = segment.getQueryableDocIds();
+ int newDocId = recordInfo.getDocId();
+ Comparable newComparisonValue = recordInfo.getComparisonValue();
+
+ // When TTL is enabled, update largestSeenComparisonValue when adding new record
+ if (_deletedKeysTTL > 0) {
+ double comparisonValue = ((Number) newComparisonValue).doubleValue();
+ _largestSeenComparisonValue.getAndUpdate(v -> Math.max(v, comparisonValue));
+ }
+
+ _primaryKeyToRecordLocationMap.compute(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (primaryKey, currentRecordLocation) -> {
+ if (currentRecordLocation != null) {
+ // Existing primary key
+ IndexSegment currentSegment = currentRecordLocation.getSegment();
+ // Update the record location when the new comparison value is greater than or equal to the current value.
+ // Update the record location when there is a tie to keep the newer record.
+ if (newComparisonValue.compareTo(currentRecordLocation.getComparisonValue()) >= 0) {
+ int currentDocId = currentRecordLocation.getDocId();
+ if (segment == currentSegment) {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ currentRecordLocation.getDistinctSegmentCount());
+ } else {
+ replaceDocId(segment, validDocIds, queryableDocIds, currentSegment, currentDocId, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue,
+ currentRecordLocation.getDistinctSegmentCount() + 1);
+ }
+ } else {
+ // Out-of-order record
+ handleOutOfOrderEvent(currentRecordLocation.getComparisonValue(), recordInfo.getComparisonValue());
+ isOutOfOrderRecord.set(true);
+ if (segment == currentSegment) {
+ return currentRecordLocation;
+ } else {
+ return new RecordLocation(currentSegment, currentRecordLocation.getDocId(),
+ currentRecordLocation.getComparisonValue(),
+ currentRecordLocation.getDistinctSegmentCount() + (_context.isDropOutOfOrderRecord() ? 0 : 1));
+ }
+ }
+ } else {
+ // New primary key
+ addDocId(segment, validDocIds, queryableDocIds, newDocId, recordInfo);
+ return new RecordLocation(segment, newDocId, newComparisonValue, 1);
+ }
+ });
+
+ updatePrimaryKeyGauge();
+ return !isOutOfOrderRecord.get();
+ }
+
+ @Override
+ protected void removeSegment(IndexSegment segment, MutableRoaringBitmap validDocIds) {
+ }
+
+ @Override
+ protected GenericRow doUpdateRecord(GenericRow record, RecordInfo recordInfo) {
+ assert _partialUpsertHandler != null;
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(recordInfo.getPrimaryKey(), _hashFunction),
+ (pk, recordLocation) -> {
+ // Read the previous record if the following conditions are met:
+ // - New record is not a DELETE record
+ // - New record is not out-of-order
+ // - Previous record is not deleted
+ 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)) {
+ _reusePreviousRow.init(currentSegment, currentDocId);
+ _partialUpsertHandler.merge(_reusePreviousRow, record, _reuseMergeResultHolder);
+ _reuseMergeResultHolder.clear();
+ }
+ }
+ return recordLocation;
+ });
+ return record;
+ }
+
+ @VisibleForTesting
+ static class RecordLocation {
+ private final IndexSegment _segment;
+ private final int _docId;
+ private final Comparable _comparisonValue;
+ private final int _distinctSegmentCount;
+
+ public RecordLocation(IndexSegment indexSegment, int docId, Comparable comparisonValue, int distinctSegmentCount) {
+ _segment = indexSegment;
+ _docId = docId;
+ _comparisonValue = comparisonValue;
+ _distinctSegmentCount = distinctSegmentCount;
+ }
+
+ public IndexSegment getSegment() {
+ return _segment;
+ }
+
+ public int getDocId() {
+ return _docId;
+ }
+
+ public Comparable getComparisonValue() {
+ return _comparisonValue;
+ }
+
+ public int getDistinctSegmentCount() {
Review Comment:
we are checking this condition in `doRemoveExpiredPrimaryKeys` that `getDistinctSegmentCount <= 1`
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1717544662
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -858,6 +858,31 @@ static void validateUpsertAndDedupConfig(TableConfig tableConfig, Schema schema)
}
}
+
+ if (upsertConfig != null && upsertConfig.isEnableDeletedKeysCompactionConsistency()) {
+ // enableConsistentDeletes shouldn't exist with metadataTTL
+ Preconditions.checkState(upsertConfig.getMetadataTTL() == 0,
+ "enableConsistentDeletes and metadataTTL shouldn't exist together for upsert table");
+
+ // enableConsistentDeletes shouldn't exist with enablePreload
+ Preconditions.checkState(!upsertConfig.isEnablePreload(),
+ "enableConsistentDeletes and enablePreload shouldn't exist together for upsert table");
+
+ // enableConsistentDeletes should exist with deletedKeysTTL
+ Preconditions.checkState(upsertConfig.getDeletedKeysTTL() > 0,
+ "enableConsistentDeletes should exist with deletedKeysTTL for upsert table");
Review Comment:
update `enableConsistentDeletes` to `enableDeletedKeysCompactionConsistency`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/TableUpsertMetadataManagerFactory.java:
##########
@@ -29,16 +29,15 @@
public class TableUpsertMetadataManagerFactory {
- private TableUpsertMetadataManagerFactory() {
- }
-
- private static final Logger LOGGER = LoggerFactory.getLogger(TableUpsertMetadataManagerFactory.class);
public static final String UPSERT_DEFAULT_METADATA_MANAGER_CLASS = "default.metadata.manager.class";
public static final String UPSERT_DEFAULT_ENABLE_SNAPSHOT = "default.enable.snapshot";
public static final String UPSERT_DEFAULT_ENABLE_PRELOAD = "default.enable.preload";
-
public static final String UPSERT_DEFAULT_ALLOW_PARTIAL_UPSERT_CONSUMPTION_DURING_COMMIT =
"default.allow.partial.upsert.consumption.during.commit";
+ private static final Logger LOGGER = LoggerFactory.getLogger(TableUpsertMetadataManagerFactory.class);
Review Comment:
looks like we don't need to change this class now that the flag is checked inside getOrCreatePartitionManager()?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1711825115
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/TableUpsertMetadataManagerFactory.java:
##########
@@ -83,8 +82,14 @@ public static TableUpsertMetadataManager create(TableConfig tableConfig,
metadataManagerClass, tableNameWithType), e);
}
} else {
- LOGGER.info("Creating ConcurrentMapTableUpsertMetadataManager for table: {}", tableNameWithType);
- metadataManager = new ConcurrentMapTableUpsertMetadataManager();
+ if (upsertConfig.isEnableConsistentDeletes()) {
Review Comment:
> hmm.. if metadataManagerClass is set, the else-branch is skipped, so the check of the new feature flag is skipped too.
in that case, the metadata-manager-class implementation should take care of using this feature flag and ensuring consistency?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1701487051
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BaseTableUpsertMetadataManager.java:
##########
@@ -81,6 +81,8 @@ public void init(TableConfig tableConfig, Schema schema, TableDataManager tableD
.setPartialUpsertHandler(partialUpsertHandler).setEnableSnapshot(enableSnapshot).setEnablePreload(enablePreload)
.setMetadataTTL(metadataTTL).setDeletedKeysTTL(deletedKeysTTL).setConsistencyMode(_consistencyMode)
.setUpsertViewRefreshIntervalMs(upsertViewRefreshIntervalMs).setTableIndexDir(tableIndexDir)
+ .setDropOutOfOrderRecord(upsertConfig.isDropOutOfOrderRecord())
+ .setEnableConsistentDeletes(upsertConfig.isEnableConsistentDeletes())
Review Comment:
Yes but won't it become non-intuitive for users to put entire `metadataManagerClass` instead of just enabling a boolean flag. Plus IMO we might make a call in future to consolidate this class with the original class and so that will make all existing tables using this `metadataManagerClass` as backward-incompatible. Better to not strictly depend on className to enable this feature.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #13347:
URL: https://github.com/apache/pinot/pull/13347#issuecomment-2158006287
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/13347?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `26 lines` in your changes missing coverage. Please review.
> Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`2bbe2bd`)](https://app.codecov.io/gh/apache/pinot/commit/2bbe2bdaf2b6e131666b21f08831d3fe0a4cb442?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 586 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/13347?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...t/ConcurrentMapPartitionUpsertMetadataManager.java](https://app.codecov.io/gh/apache/pinot/pull/13347?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fupsert%2FConcurrentMapPartitionUpsertMetadataManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQ29uY3VycmVudE1hcFBhcnRpdGlvblVwc2VydE1ldGFkYXRhTWFuYWdlci5qYXZh) | 0.00% | [26 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13347?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #13347 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 2470 +34
Lines 133233 136151 +2918
Branches 20636 21141 +505
=============================================
- Hits 82274 0 -82274
- Misses 44911 136151 +91240
+ Partials 6048 0 -6048
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/13347/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/13347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/13347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/13347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/13347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/13347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/13347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.63%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/13347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/13347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-27.73%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/13347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.76%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/13347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/13347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/13347/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/13347?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1712724090
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/TableUpsertMetadataManagerFactory.java:
##########
@@ -83,8 +82,14 @@ public static TableUpsertMetadataManager create(TableConfig tableConfig,
metadataManagerClass, tableNameWithType), e);
}
} else {
- LOGGER.info("Creating ConcurrentMapTableUpsertMetadataManager for table: {}", tableNameWithType);
- metadataManager = new ConcurrentMapTableUpsertMetadataManager();
+ if (upsertConfig.isEnableConsistentDeletes()) {
Review Comment:
Yes thanks Jackie for this suggestion! Looks neat now.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Ensure upsert deletion consistency when enabled with compaction flow [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13347:
URL: https://github.com/apache/pinot/pull/13347#discussion_r1710012624
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapTableUpsertMetadataManagerForConsistentDeletes.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.upsert;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.pinot.segment.spi.SegmentContext;
+
+
+/**
+ * Implementation of {@link TableUpsertMetadataManager} that is backed by a {@link ConcurrentHashMap} and ensures
+ * consistent deletions. This should be used when the table is configured with 'consistentDeletes' set to true.
+ */
+public class ConcurrentMapTableUpsertMetadataManagerForConsistentDeletes extends BaseTableUpsertMetadataManager {
Review Comment:
Other than `doUpdateRecord` and `getNumPrimaryKeys` none of the methods are same afaik.. since RecordLocation is a static class in each manager it makes it a bit unreadable when creating new RecordLocation in this class as it will have to be prefixed by this metadata-manager-class name everytime then..
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org