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/10/10 16:53:02 UTC
[PR] [Do not review] add support for retention on deleted upsert keys [pinot]
tibrewalpratik17 opened a new pull request, #11779:
URL: https://github.com/apache/pinot/pull/11779
labels:
- feature
Created this draft PR to just give a design view of supporting retention for deleted upsert primary keys which we are discussing in #11736.
TODO:
- Add config validations
- Add UTs
- Yet to test
--
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] [Do not review] add support for retention on deleted upsert keys [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on code in PR #11779:
URL: https://github.com/apache/pinot/pull/11779#discussion_r1357214183
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -129,20 +133,23 @@ public void addSegment(ImmutableSegment segment) {
return;
}
Preconditions.checkArgument(segment instanceof ImmutableSegmentImpl,
- "Got unsupported segment implementation: {} for segment: {}, table: {}", segment.getClass(), segmentName,
- _tableNameWithType);
+ "Got unsupported segment implementation: {} for segment: {}, table: {}", segment.getClass(),
+ segmentName, _tableNameWithType);
ImmutableSegmentImpl immutableSegment = (ImmutableSegmentImpl) segment;
// Skip adding segment that has max comparison value smaller than (largestSeenComparisonValue - TTL)
- if (_largestSeenComparisonValue > 0) {
+ // if _enableMetadataTTLForDeletedRecord is true then update _largestSeenComparisonValue to max comparison value
+ // and do not skip adding segment
+ if (_largestSeenComparisonValue > 0 || _enableMetadataTTLForDeletedRecord) {
Preconditions.checkState(_enableSnapshot, "Upsert TTL must have snapshot enabled");
Preconditions.checkState(_comparisonColumns.size() == 1,
"Upsert TTL does not work with multiple comparison columns");
- // TODO: Support deletion for TTL. Need to construct queryableDocIds when adding segments out of TTL.
- Preconditions.checkState(_deleteRecordColumn == null, "Upsert TTL doesn't work with record deletion");
+ Preconditions.checkState(_deleteRecordColumn == null || _enableMetadataTTLForDeletedRecord,
+ "Upsert TTL doesn't work with record deletion without enableMetadataTTLForDeletedRecord flag");
Review Comment:
thanks @tibrewalpratik17 i will take a look this week.
--
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] [Do not review] add support for retention on deleted upsert keys [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on code in PR #11779:
URL: https://github.com/apache/pinot/pull/11779#discussion_r1355858602
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -129,20 +133,23 @@ public void addSegment(ImmutableSegment segment) {
return;
}
Preconditions.checkArgument(segment instanceof ImmutableSegmentImpl,
- "Got unsupported segment implementation: {} for segment: {}, table: {}", segment.getClass(), segmentName,
- _tableNameWithType);
+ "Got unsupported segment implementation: {} for segment: {}, table: {}", segment.getClass(),
+ segmentName, _tableNameWithType);
ImmutableSegmentImpl immutableSegment = (ImmutableSegmentImpl) segment;
// Skip adding segment that has max comparison value smaller than (largestSeenComparisonValue - TTL)
- if (_largestSeenComparisonValue > 0) {
+ // if _enableMetadataTTLForDeletedRecord is true then update _largestSeenComparisonValue to max comparison value
+ // and do not skip adding segment
+ if (_largestSeenComparisonValue > 0 || _enableMetadataTTLForDeletedRecord) {
Preconditions.checkState(_enableSnapshot, "Upsert TTL must have snapshot enabled");
Preconditions.checkState(_comparisonColumns.size() == 1,
"Upsert TTL does not work with multiple comparison columns");
- // TODO: Support deletion for TTL. Need to construct queryableDocIds when adding segments out of TTL.
- Preconditions.checkState(_deleteRecordColumn == null, "Upsert TTL doesn't work with record deletion");
+ Preconditions.checkState(_deleteRecordColumn == null || _enableMetadataTTLForDeletedRecord,
+ "Upsert TTL doesn't work with record deletion without enableMetadataTTLForDeletedRecord flag");
Review Comment:
Hi @tibrewalpratik17 this is a todo, we should mentioned upsert ttl with deletion is `to be implemented` instead mentioned that `Upsert TTL doesn't work with record deletion without enableMetadataTTLForDeletedRecord flag`. Let me take create a patch to address this todo, then you can add this `enableMetadataTTLForDeletedRecord` 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
Re: [PR] [Do not review] add support for retention on deleted upsert keys [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on PR #11779:
URL: https://github.com/apache/pinot/pull/11779#issuecomment-1842150780
Completed in #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] [Do not review] add support for retention on deleted upsert keys [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #11779:
URL: https://github.com/apache/pinot/pull/11779#discussion_r1356593308
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -129,20 +133,23 @@ public void addSegment(ImmutableSegment segment) {
return;
}
Preconditions.checkArgument(segment instanceof ImmutableSegmentImpl,
- "Got unsupported segment implementation: {} for segment: {}, table: {}", segment.getClass(), segmentName,
- _tableNameWithType);
+ "Got unsupported segment implementation: {} for segment: {}, table: {}", segment.getClass(),
+ segmentName, _tableNameWithType);
ImmutableSegmentImpl immutableSegment = (ImmutableSegmentImpl) segment;
// Skip adding segment that has max comparison value smaller than (largestSeenComparisonValue - TTL)
- if (_largestSeenComparisonValue > 0) {
+ // if _enableMetadataTTLForDeletedRecord is true then update _largestSeenComparisonValue to max comparison value
+ // and do not skip adding segment
+ if (_largestSeenComparisonValue > 0 || _enableMetadataTTLForDeletedRecord) {
Preconditions.checkState(_enableSnapshot, "Upsert TTL must have snapshot enabled");
Preconditions.checkState(_comparisonColumns.size() == 1,
"Upsert TTL does not work with multiple comparison columns");
- // TODO: Support deletion for TTL. Need to construct queryableDocIds when adding segments out of TTL.
- Preconditions.checkState(_deleteRecordColumn == null, "Upsert TTL doesn't work with record deletion");
+ Preconditions.checkState(_deleteRecordColumn == null || _enableMetadataTTLForDeletedRecord,
+ "Upsert TTL doesn't work with record deletion without enableMetadataTTLForDeletedRecord flag");
Review Comment:
Sure I see you have raised #11791 to address this. Will rebase once merged.
@deemoliu do you see any other issue with this overall design or are we good to move forward with this?
--
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] [Do not review] add support for retention on deleted upsert keys [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11779:
URL: https://github.com/apache/pinot/pull/11779#issuecomment-1755927451
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11779?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11779](https://app.codecov.io/gh/apache/pinot/pull/11779?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (0ff1374) into [master](https://app.codecov.io/gh/apache/pinot/commit/873d76e9a216cb496168eb62f02fc1ed849b7ebb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (873d76e) will **decrease** coverage by `48.70%`.
> Report is 6 commits behind head on master.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11779 +/- ##
=============================================
- Coverage 63.10% 14.41% -48.70%
+ Complexity 1118 201 -917
=============================================
Files 2342 2342
Lines 125916 125932 +16
Branches 19370 19375 +5
=============================================
- Hits 79461 18154 -61307
- Misses 40808 106240 +65432
+ Partials 5647 1538 -4109
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11779/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/11779/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/11779/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/11779/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/11779/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-17](https://app.codecov.io/gh/apache/pinot/pull/11779/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-20](https://app.codecov.io/gh/apache/pinot/pull/11779/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.41% <0.00%> (-48.55%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11779/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.41% <0.00%> (-48.70%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11779/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.41% <0.00%> (-48.69%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11779/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/11779/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.41% <0.00%> (-0.02%)` | :arrow_down: |
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.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/11779?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...psert/ConcurrentMapTableUpsertMetadataManager.java](https://app.codecov.io/gh/apache/pinot/pull/11779?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQ29uY3VycmVudE1hcFRhYmxlVXBzZXJ0TWV0YWRhdGFNYW5hZ2VyLmphdmE=) | `0.00% <ø> (-33.34%)` | :arrow_down: |
| [...t/local/upsert/BaseTableUpsertMetadataManager.java](https://app.codecov.io/gh/apache/pinot/pull/11779?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQmFzZVRhYmxlVXBzZXJ0TWV0YWRhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-76.54%)` | :arrow_down: |
| [...rg/apache/pinot/spi/config/table/UpsertConfig.java](https://app.codecov.io/gh/apache/pinot/pull/11779?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1Vwc2VydENvbmZpZy5qYXZh) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [...cal/upsert/BasePartitionUpsertMetadataManager.java](https://app.codecov.io/gh/apache/pinot/pull/11779?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQmFzZVBhcnRpdGlvblVwc2VydE1ldGFkYXRhTWFuYWdlci5qYXZh) | `0.00% <0.00%> (-40.62%)` | :arrow_down: |
| [...t/ConcurrentMapPartitionUpsertMetadataManager.java](https://app.codecov.io/gh/apache/pinot/pull/11779?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQ29uY3VycmVudE1hcFBhcnRpdGlvblVwc2VydE1ldGFkYXRhTWFuYWdlci5qYXZh) | `0.00% <0.00%> (-85.00%)` | :arrow_down: |
... and [1510 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11779/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] [Do not review] add support for retention on deleted upsert keys [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 closed pull request #11779: [Do not review] add support for retention on deleted upsert keys
URL: https://github.com/apache/pinot/pull/11779
--
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