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 2023/11/21 21:38:06 UTC
[PR] Add support for retention on deleted keys of upsert tables [pinot]
tibrewalpratik17 opened a new pull request, #12037:
URL: https://github.com/apache/pinot/pull/12037
label
- feature
- upsert
This patch adds a config in upsert - `deletedKeysTTL` which when set will remove deleted keys from in-memory hashmap and mark the validDocID as invalid after the deletedKeysTTL threshold period. This will help in realising in-memory cost for the keys that are deleted.
Relates to #11736
--
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] Add support for retention on deleted keys of upsert tables [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #12037:
URL: https://github.com/apache/pinot/pull/12037#discussion_r1416222824
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -157,15 +160,20 @@ public void addSegment(ImmutableSegment segment) {
"Got unsupported segment implementation: {} for segment: {}, table: {}", segment.getClass(), segmentName,
_tableNameWithType);
ImmutableSegmentImpl immutableSegment = (ImmutableSegmentImpl) segment;
+ double maxComparisonValue =
Review Comment:
Thanks for pointing this out! 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] Add support for retention on deleted keys of upsert tables [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #12037:
URL: https://github.com/apache/pinot/pull/12037#discussion_r1416222326
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerMeter.java:
##########
@@ -48,6 +48,7 @@ public enum ServerMeter implements AbstractMetrics.Meter {
PARTIAL_UPSERT_OUT_OF_ORDER("rows", false),
PARTIAL_UPSERT_KEYS_NOT_REPLACED("rows", false),
UPSERT_OUT_OF_ORDER("rows", false),
+ DELETED_KEYS_TTL_PRIMARY_KEY_REMOVED("rows", false),
Review Comment:
Added 2 meters.
--
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] Add support for retention on deleted keys of upsert tables [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12037:
URL: https://github.com/apache/pinot/pull/12037#issuecomment-1821786635
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12037?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `59 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`2c88c4f`)](https://app.codecov.io/gh/apache/pinot/commit/2c88c4f74d9e74984b284298b76a855f42e7cc61?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.63% compared to head [(`1adc5f0`)](https://app.codecov.io/gh/apache/pinot/pull/12037?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 46.75%.
> Report is 2 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12037?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/12037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQ29uY3VycmVudE1hcFBhcnRpdGlvblVwc2VydE1ldGFkYXRhTWFuYWdlci5qYXZh) | 0.00% | [28 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...cal/upsert/BasePartitionUpsertMetadataManager.java](https://app.codecov.io/gh/apache/pinot/pull/12037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQmFzZVBhcnRpdGlvblVwc2VydE1ldGFkYXRhTWFuYWdlci5qYXZh) | 0.00% | [18 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...he/pinot/segment/local/utils/TableConfigUtils.java](https://app.codecov.io/gh/apache/pinot/pull/12037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9UYWJsZUNvbmZpZ1V0aWxzLmphdmE=) | 0.00% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...t/local/upsert/BaseTableUpsertMetadataManager.java](https://app.codecov.io/gh/apache/pinot/pull/12037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQmFzZVRhYmxlVXBzZXJ0TWV0YWRhdGFNYW5hZ2VyLmphdmE=) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...a/manager/realtime/RealtimeSegmentDataManager.java](https://app.codecov.io/gh/apache/pinot/pull/12037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVTZWdtZW50RGF0YU1hbmFnZXIuamF2YQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12037?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 #12037 +/- ##
=============================================
- Coverage 61.63% 46.75% -14.88%
- Complexity 207 944 +737
=============================================
Files 2385 1787 -598
Lines 129519 93951 -35568
Branches 20043 15193 -4850
=============================================
- Hits 79824 43926 -35898
- Misses 43882 46900 +3018
+ Partials 5813 3125 -2688
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12037/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/12037/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/12037/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12037/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/12037/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12037/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/12037/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.75% <6.34%> (-14.76%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12037/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.75% <6.34%> (-14.85%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12037/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12037/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.75% <6.34%> (-14.88%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12037/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.75% <6.34%> (-14.88%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12037/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.75% <6.34%> (-0.23%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12037/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/12037?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] Add support for retention on deleted keys of upsert tables [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #12037:
URL: https://github.com/apache/pinot/pull/12037#discussion_r1410611896
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -240,6 +243,26 @@ public void doRemoveExpiredPrimaryKeys() {
persistWatermark(_largestSeenComparisonValue);
}
+ @Override
+ public void doRemoveExpiredDeletedKeys() {
+ double threshold = _largestSeenComparisonValue - _deletedKeysTTL;
+ AtomicInteger numDeletedKeys = new AtomicInteger();
+ _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
Review Comment:
Moved to `doRemoveExpiredPrimaryKeys` so we are looping once. Also added metrics for tracking duration of this operation and number of keys deleted.
--
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] Add support for retention on deleted keys of upsert tables [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12037:
URL: https://github.com/apache/pinot/pull/12037#discussion_r1410119462
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -323,11 +347,13 @@ static class RecordLocation {
private final IndexSegment _segment;
private final int _docId;
private final Comparable _comparisonValue;
+ private final boolean _isDeletedRecord;
Review Comment:
From the queryableDocIds, we should be able to tell whether a primary key is deleted
--
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] Add support for retention on deleted keys of upsert tables [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #12037:
URL: https://github.com/apache/pinot/pull/12037
--
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] Add support for retention on deleted keys of upsert tables [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #12037:
URL: https://github.com/apache/pinot/pull/12037#discussion_r1410611230
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -323,11 +347,13 @@ static class RecordLocation {
private final IndexSegment _segment;
private final int _docId;
private final Comparable _comparisonValue;
+ private final boolean _isDeletedRecord;
Review Comment:
Good point! Removed this field from RecordLocation.
--
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] Add support for retention on deleted keys of upsert tables [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12037:
URL: https://github.com/apache/pinot/pull/12037#discussion_r1410118166
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -323,11 +347,13 @@ static class RecordLocation {
private final IndexSegment _segment;
private final int _docId;
private final Comparable _comparisonValue;
+ private final boolean _isDeletedRecord;
Review Comment:
Ideally we don't want to add field here since this will add 1 byte per entry. Because of memory alignment, currently each `RecordLocation` is 32 bytes, and this will make it 40 bytes.
--
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] Add support for retention on deleted keys of upsert tables [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12037:
URL: https://github.com/apache/pinot/pull/12037#discussion_r1414717992
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -731,6 +731,21 @@ static void validateUpsertAndDedupConfig(TableConfig tableConfig, Schema schema)
"The delete record column must be a single-valued BOOLEAN column");
}
+ double deletedKeysTTL = upsertConfig.getDeletedKeysTTL();
Review Comment:
Shall we move this along with `metadataTTL` validation to avoid duplicate code on `comparisonColumns` check?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -231,13 +233,49 @@ protected void removeSegment(IndexSegment segment, MutableRoaringBitmap validDoc
@Override
public void doRemoveExpiredPrimaryKeys() {
- double threshold = _largestSeenComparisonValue - _metadataTTL;
+ AtomicInteger numDeletedKeys = new AtomicInteger();
+ long startTime = System.currentTimeMillis();
+ double metadataTTLKeysThreshold;
+ if (_metadataTTL > 0) {
+ metadataTTLKeysThreshold = _largestSeenComparisonValue - _metadataTTL;
+ } else {
+ metadataTTLKeysThreshold = Double.MIN_VALUE;
+ }
+
+ double deletedKeysThreshold;
+
+ if (_deletedKeysTTL > 0) {
+ deletedKeysThreshold = _largestSeenComparisonValue - _deletedKeysTTL;
+ } else {
+ deletedKeysThreshold = Double.MIN_VALUE;
+ }
_primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (((Number) recordLocation.getComparisonValue()).doubleValue() < threshold) {
+ if (_metadataTTL > 0 && ((Number) recordLocation.getComparisonValue()).doubleValue() < metadataTTLKeysThreshold) {
_primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ numDeletedKeys.getAndIncrement();
+ } else if (_deletedKeysTTL > 0
+ && ((Number) recordLocation.getComparisonValue()).doubleValue() < deletedKeysThreshold) {
+ 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())) {
Review Comment:
(minor) `currentQueryableDocIds != null` should be redundant
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java:
##########
@@ -47,7 +47,10 @@ public enum ServerTimer implements AbstractMetrics.Timer {
SEGMENT_UPLOAD_TIME_MS("milliseconds", false),
TOTAL_CPU_TIME_NS("nanoseconds", false, "Total query cost (thread cpu time + system "
- + "activities cpu time + response serialization cpu time) for query processing on server.");
+ + "activities cpu time + response serialization cpu time) for query processing on server."),
+
+ EXPIRED_PRIMARY_KEYS_DELETION_TIME_MS("milliseconds", false, "Total time taken to delete expired primary keys based "
Review Comment:
(minor) Suggest renaming it to `UPSERT_REMOVE_EXPIRED_PRIMARY_KEYS_TIME_MS` to match the function name.
As a follow up, we can add timer for other operations as well
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -231,13 +233,49 @@ protected void removeSegment(IndexSegment segment, MutableRoaringBitmap validDoc
@Override
public void doRemoveExpiredPrimaryKeys() {
- double threshold = _largestSeenComparisonValue - _metadataTTL;
+ AtomicInteger numDeletedKeys = new AtomicInteger();
+ long startTime = System.currentTimeMillis();
+ double metadataTTLKeysThreshold;
+ if (_metadataTTL > 0) {
+ metadataTTLKeysThreshold = _largestSeenComparisonValue - _metadataTTL;
+ } else {
+ metadataTTLKeysThreshold = Double.MIN_VALUE;
+ }
+
+ double deletedKeysThreshold;
+
+ if (_deletedKeysTTL > 0) {
+ deletedKeysThreshold = _largestSeenComparisonValue - _deletedKeysTTL;
+ } else {
+ deletedKeysThreshold = Double.MIN_VALUE;
+ }
_primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (((Number) recordLocation.getComparisonValue()).doubleValue() < threshold) {
+ if (_metadataTTL > 0 && ((Number) recordLocation.getComparisonValue()).doubleValue() < metadataTTLKeysThreshold) {
Review Comment:
(minor) Cache `((Number) recordLocation.getComparisonValue()).doubleValue()`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -231,13 +233,49 @@ protected void removeSegment(IndexSegment segment, MutableRoaringBitmap validDoc
@Override
public void doRemoveExpiredPrimaryKeys() {
- double threshold = _largestSeenComparisonValue - _metadataTTL;
+ AtomicInteger numDeletedKeys = new AtomicInteger();
+ long startTime = System.currentTimeMillis();
+ double metadataTTLKeysThreshold;
+ if (_metadataTTL > 0) {
+ metadataTTLKeysThreshold = _largestSeenComparisonValue - _metadataTTL;
+ } else {
+ metadataTTLKeysThreshold = Double.MIN_VALUE;
+ }
+
+ double deletedKeysThreshold;
+
+ if (_deletedKeysTTL > 0) {
+ deletedKeysThreshold = _largestSeenComparisonValue - _deletedKeysTTL;
+ } else {
+ deletedKeysThreshold = Double.MIN_VALUE;
+ }
_primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (((Number) recordLocation.getComparisonValue()).doubleValue() < threshold) {
+ if (_metadataTTL > 0 && ((Number) recordLocation.getComparisonValue()).doubleValue() < metadataTTLKeysThreshold) {
_primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ numDeletedKeys.getAndIncrement();
+ } else if (_deletedKeysTTL > 0
+ && ((Number) recordLocation.getComparisonValue()).doubleValue() < deletedKeysThreshold) {
+ 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());
+ numDeletedKeys.getAndIncrement();
+ }
}
});
- persistWatermark(_largestSeenComparisonValue);
+ if (_metadataTTL > 0) {
+ persistWatermark(_largestSeenComparisonValue);
+ }
+ long duration = System.currentTimeMillis() - startTime;
+ int numDeletedTTLKeys = numDeletedKeys.get();
+ if (numDeletedTTLKeys > 0) {
+ _logger.info("Deleted {} primary deleted keys in the table {}", numDeletedTTLKeys, _tableNameWithType);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.DELETED_KEYS_TTL_PRIMARY_KEY_REMOVED,
+ numDeletedTTLKeys);
+ }
+ _serverMetrics.addTimedTableValue(_tableNameWithType, ServerTimer.EXPIRED_PRIMARY_KEYS_DELETION_TIME_MS, duration,
Review Comment:
(minor) This can be added to `BasePartitionUpsertMetadataManager` so that other implementation can also record the time
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -157,15 +160,20 @@ public void addSegment(ImmutableSegment segment) {
"Got unsupported segment implementation: {} for segment: {}, table: {}", segment.getClass(), segmentName,
_tableNameWithType);
ImmutableSegmentImpl immutableSegment = (ImmutableSegmentImpl) segment;
+ double maxComparisonValue =
Review Comment:
(MAJOR) We cannot read value here because there is no guarantee the comparison column is number when TTL is not enabled
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerMeter.java:
##########
@@ -48,6 +48,7 @@ public enum ServerMeter implements AbstractMetrics.Meter {
PARTIAL_UPSERT_OUT_OF_ORDER("rows", false),
PARTIAL_UPSERT_KEYS_NOT_REPLACED("rows", false),
UPSERT_OUT_OF_ORDER("rows", false),
+ DELETED_KEYS_TTL_PRIMARY_KEY_REMOVED("rows", false),
Review Comment:
+1 on adding this meter. Shall we keep removed keys from metadata TTL and delete TTL separately?
--
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] Add support for retention on deleted keys of upsert tables [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12037:
URL: https://github.com/apache/pinot/pull/12037#discussion_r1410118166
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -323,11 +347,13 @@ static class RecordLocation {
private final IndexSegment _segment;
private final int _docId;
private final Comparable _comparisonValue;
+ private final boolean _isDeletedRecord;
Review Comment:
Ideally we don't want to add field here since this will add 1 byte per entry. Currently each `RecordLocation` is 4 bytes, and this will make it 5 bytes.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -240,6 +243,26 @@ public void doRemoveExpiredPrimaryKeys() {
persistWatermark(_largestSeenComparisonValue);
}
+ @Override
+ public void doRemoveExpiredDeletedKeys() {
+ double threshold = _largestSeenComparisonValue - _deletedKeysTTL;
+ AtomicInteger numDeletedKeys = new AtomicInteger();
+ _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
Review Comment:
Looping over all entries is a very expensive operation, so we should either only filter all deleted records (using bitmap to get them), or combine it with `doRemoveExpiredPrimaryKeys()` and only loop once
--
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] Add support for retention on deleted keys of upsert tables [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #12037:
URL: https://github.com/apache/pinot/pull/12037#discussion_r1416264208
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -231,13 +233,49 @@ protected void removeSegment(IndexSegment segment, MutableRoaringBitmap validDoc
@Override
public void doRemoveExpiredPrimaryKeys() {
- double threshold = _largestSeenComparisonValue - _metadataTTL;
+ AtomicInteger numDeletedKeys = new AtomicInteger();
+ long startTime = System.currentTimeMillis();
+ double metadataTTLKeysThreshold;
+ if (_metadataTTL > 0) {
+ metadataTTLKeysThreshold = _largestSeenComparisonValue - _metadataTTL;
+ } else {
+ metadataTTLKeysThreshold = Double.MIN_VALUE;
+ }
+
+ double deletedKeysThreshold;
+
+ if (_deletedKeysTTL > 0) {
+ deletedKeysThreshold = _largestSeenComparisonValue - _deletedKeysTTL;
+ } else {
+ deletedKeysThreshold = Double.MIN_VALUE;
+ }
_primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (((Number) recordLocation.getComparisonValue()).doubleValue() < threshold) {
+ if (_metadataTTL > 0 && ((Number) recordLocation.getComparisonValue()).doubleValue() < metadataTTLKeysThreshold) {
_primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ numDeletedKeys.getAndIncrement();
+ } else if (_deletedKeysTTL > 0
+ && ((Number) recordLocation.getComparisonValue()).doubleValue() < deletedKeysThreshold) {
+ 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())) {
Review Comment:
Will it not throw NPE in `!currentQueryableDocIds.contains(recordLocation.getDocId()))` ?
--
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] Add support for retention on deleted keys of upsert tables [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12037:
URL: https://github.com/apache/pinot/pull/12037#discussion_r1416400404
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -231,13 +233,49 @@ protected void removeSegment(IndexSegment segment, MutableRoaringBitmap validDoc
@Override
public void doRemoveExpiredPrimaryKeys() {
- double threshold = _largestSeenComparisonValue - _metadataTTL;
+ AtomicInteger numDeletedKeys = new AtomicInteger();
+ long startTime = System.currentTimeMillis();
+ double metadataTTLKeysThreshold;
+ if (_metadataTTL > 0) {
+ metadataTTLKeysThreshold = _largestSeenComparisonValue - _metadataTTL;
+ } else {
+ metadataTTLKeysThreshold = Double.MIN_VALUE;
+ }
+
+ double deletedKeysThreshold;
+
+ if (_deletedKeysTTL > 0) {
+ deletedKeysThreshold = _largestSeenComparisonValue - _deletedKeysTTL;
+ } else {
+ deletedKeysThreshold = Double.MIN_VALUE;
+ }
_primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
- if (((Number) recordLocation.getComparisonValue()).doubleValue() < threshold) {
+ if (_metadataTTL > 0 && ((Number) recordLocation.getComparisonValue()).doubleValue() < metadataTTLKeysThreshold) {
_primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
+ numDeletedKeys.getAndIncrement();
+ } else if (_deletedKeysTTL > 0
+ && ((Number) recordLocation.getComparisonValue()).doubleValue() < deletedKeysThreshold) {
+ 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())) {
Review Comment:
It should always exist, but doesn't hurt to check
--
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