You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/07/23 07:45:06 UTC
[GitHub] [pinot] Jackie-Jiang opened a new pull request, #9095: Enhance upsert metadata handling
Jackie-Jiang opened a new pull request, #9095:
URL: https://github.com/apache/pinot/pull/9095
Make the following enhancement to the upsert metadata manager:
- Add replace segment support
- Log error and emit metric (`PARTIAL_UPSERT_ROWS_NOT_REPLACED`) for segment not fully replaced, which can potentially cause inconsistency between replicas for partial upsert table
- Remove the remaining primary keys from the replaced segment immediately so that new consuming segment is not affected
- Handle empty segment properly
- Enhance the log to log the table name, partition id and primary key count
- Clean up the code and move the upsert related logic into the metadata manager, such as creating the record info iterator
- In `IndexSegment`, replace `getPrimaryKey()` with `getValue()` which is more general and can be used to read the comparison column as well
- Fix the issue of using BYTES as primary key
## Release Notes
Added a metric `PARTIAL_UPSERT_ROWS_NOT_REPLACED` to track the rows not replaced for partial upsert table
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9095: Enhance upsert metadata handling
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9095:
URL: https://github.com/apache/pinot/pull/9095#discussion_r928164534
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -208,11 +275,82 @@ public void addRecord(IndexSegment segment, RecordInfo recordInfo) {
return new RecordLocation(segment, recordInfo.getDocId(), recordInfo.getComparisonValue());
}
});
+
// Update metrics
_serverMetrics.setValueOfPartitionGauge(_tableNameWithType, _partitionId, ServerGauge.UPSERT_PRIMARY_KEYS_COUNT,
_primaryKeyToRecordLocationMap.size());
}
+ /**
+ * Replaces the upsert metadata for the old segment with the new immutable segment.
+ */
+ public void replaceSegment(ImmutableSegment newSegment, IndexSegment oldSegment) {
+ String segmentName = newSegment.getSegmentName();
+ Preconditions.checkArgument(segmentName.equals(oldSegment.getSegmentName()),
+ "Cannot replace segment with different name for table: {}, old segment: {}, new segment: {}",
+ _tableNameWithType, oldSegment.getSegmentName(), segmentName);
+ _logger.info("Replacing upsert metadata for {} segment: {}",
+ oldSegment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName);
+
+ addSegment(newSegment);
+
+ MutableRoaringBitmap validDocIds =
+ oldSegment.getValidDocIds() != null ? oldSegment.getValidDocIds().getMutableRoaringBitmap() : null;
+ if (validDocIds != null && !validDocIds.isEmpty()) {
+ int numDocsNotReplaced = validDocIds.getCardinality();
+ if (_partialUpsertHandler != null) {
+ _logger.error("Got {} primary keys not replaced when replacing segment: {} for partial upsert table. This can "
+ + "potentially cause inconsistency between replicas", numDocsNotReplaced, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.PARTIAL_UPSERT_ROWS_NOT_REPLACED,
+ numDocsNotReplaced);
+ } else {
+ _logger.info("Got {} primary keys not replaced when replacing segment: {}", numDocsNotReplaced, segmentName);
+ }
+ removeSegment(oldSegment);
+ }
+
+ _logger.info("Finish replacing upsert metadata for segment: {}", segmentName);
+ }
+
+ /**
+ * Removes the upsert metadata for the given segment.
+ */
+ public void removeSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing upsert metadata for segment: {}, primary key count: {}", segmentName,
+ _primaryKeyToRecordLocationMap.size());
+
+ MutableRoaringBitmap validDocIds =
+ segment.getValidDocIds() != null ? segment.getValidDocIds().getMutableRoaringBitmap() : null;
+ if (validDocIds == null || validDocIds.isEmpty()) {
+ _logger.info("Skipping removing upsert metadata for segment without valid docs: {}", segmentName);
+ return;
+ }
+
+ _logger.info("Trying to remove {} primary keys from upsert metadata for segment: {}", validDocIds.getCardinality(),
Review Comment:
This can be useful, and shouldn't be logged frequently, so prefer keeping it at info level.
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #9095: Enhance upsert metadata handling
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9095:
URL: https://github.com/apache/pinot/pull/9095#discussion_r928156098
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -208,11 +275,82 @@ public void addRecord(IndexSegment segment, RecordInfo recordInfo) {
return new RecordLocation(segment, recordInfo.getDocId(), recordInfo.getComparisonValue());
}
});
+
// Update metrics
_serverMetrics.setValueOfPartitionGauge(_tableNameWithType, _partitionId, ServerGauge.UPSERT_PRIMARY_KEYS_COUNT,
_primaryKeyToRecordLocationMap.size());
}
+ /**
+ * Replaces the upsert metadata for the old segment with the new immutable segment.
+ */
+ public void replaceSegment(ImmutableSegment newSegment, IndexSegment oldSegment) {
+ String segmentName = newSegment.getSegmentName();
+ Preconditions.checkArgument(segmentName.equals(oldSegment.getSegmentName()),
+ "Cannot replace segment with different name for table: {}, old segment: {}, new segment: {}",
+ _tableNameWithType, oldSegment.getSegmentName(), segmentName);
+ _logger.info("Replacing upsert metadata for {} segment: {}",
+ oldSegment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName);
+
+ addSegment(newSegment);
+
+ MutableRoaringBitmap validDocIds =
+ oldSegment.getValidDocIds() != null ? oldSegment.getValidDocIds().getMutableRoaringBitmap() : null;
+ if (validDocIds != null && !validDocIds.isEmpty()) {
+ int numDocsNotReplaced = validDocIds.getCardinality();
+ if (_partialUpsertHandler != null) {
+ _logger.error("Got {} primary keys not replaced when replacing segment: {} for partial upsert table. This can "
Review Comment:
Also, is error log necessary here? since we are already doing `removeSegment` just after this which would anyway remove these docIds
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #9095: Enhance upsert metadata handling
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9095:
URL: https://github.com/apache/pinot/pull/9095#discussion_r928155281
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -208,11 +275,82 @@ public void addRecord(IndexSegment segment, RecordInfo recordInfo) {
return new RecordLocation(segment, recordInfo.getDocId(), recordInfo.getComparisonValue());
}
});
+
// Update metrics
_serverMetrics.setValueOfPartitionGauge(_tableNameWithType, _partitionId, ServerGauge.UPSERT_PRIMARY_KEYS_COUNT,
_primaryKeyToRecordLocationMap.size());
}
+ /**
+ * Replaces the upsert metadata for the old segment with the new immutable segment.
+ */
+ public void replaceSegment(ImmutableSegment newSegment, IndexSegment oldSegment) {
+ String segmentName = newSegment.getSegmentName();
+ Preconditions.checkArgument(segmentName.equals(oldSegment.getSegmentName()),
+ "Cannot replace segment with different name for table: {}, old segment: {}, new segment: {}",
+ _tableNameWithType, oldSegment.getSegmentName(), segmentName);
+ _logger.info("Replacing upsert metadata for {} segment: {}",
+ oldSegment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName);
+
+ addSegment(newSegment);
+
+ MutableRoaringBitmap validDocIds =
+ oldSegment.getValidDocIds() != null ? oldSegment.getValidDocIds().getMutableRoaringBitmap() : null;
+ if (validDocIds != null && !validDocIds.isEmpty()) {
+ int numDocsNotReplaced = validDocIds.getCardinality();
+ if (_partialUpsertHandler != null) {
+ _logger.error("Got {} primary keys not replaced when replacing segment: {} for partial upsert table. This can "
+ + "potentially cause inconsistency between replicas", numDocsNotReplaced, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.PARTIAL_UPSERT_ROWS_NOT_REPLACED,
+ numDocsNotReplaced);
+ } else {
+ _logger.info("Got {} primary keys not replaced when replacing segment: {}", numDocsNotReplaced, segmentName);
+ }
+ removeSegment(oldSegment);
+ }
+
+ _logger.info("Finish replacing upsert metadata for segment: {}", segmentName);
+ }
+
+ /**
+ * Removes the upsert metadata for the given segment.
+ */
+ public void removeSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing upsert metadata for segment: {}, primary key count: {}", segmentName,
+ _primaryKeyToRecordLocationMap.size());
+
+ MutableRoaringBitmap validDocIds =
+ segment.getValidDocIds() != null ? segment.getValidDocIds().getMutableRoaringBitmap() : null;
+ if (validDocIds == null || validDocIds.isEmpty()) {
+ _logger.info("Skipping removing upsert metadata for segment without valid docs: {}", segmentName);
+ return;
+ }
+
+ _logger.info("Trying to remove {} primary keys from upsert metadata for segment: {}", validDocIds.getCardinality(),
+ segmentName);
+ PrimaryKey primaryKey = new PrimaryKey(new Object[_primaryKeyColumns.size()]);
+ PeekableIntIterator iterator = validDocIds.getIntIterator();
+ while (iterator.hasNext()) {
+ int docId = iterator.next();
+ getPrimaryKey(segment, docId, primaryKey);
+ _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(primaryKey, _hashFunction),
+ (pk, recordLocation) -> {
+ if (recordLocation.getSegment() == segment) {
+ return null;
+ }
+ return recordLocation;
+ });
+ }
+
+ // Update metrics
+ int numPrimaryKeys = _primaryKeyToRecordLocationMap.size();
+ _serverMetrics.setValueOfPartitionGauge(_tableNameWithType, _partitionId, ServerGauge.UPSERT_PRIMARY_KEYS_COUNT,
+ numPrimaryKeys);
+
+ _logger.info("Finish removing upsert metadata for segment: {}, primary key count: {}", segmentName, numPrimaryKeys);
Review Comment:
```suggestion
_logger.info("Finished removing primary keys from upsert metadata for segment: {}, current primary key count: {}", segmentName, numPrimaryKeys);
```
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #9095: Enhance upsert metadata handling
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9095:
URL: https://github.com/apache/pinot/pull/9095#discussion_r928155890
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -208,11 +275,82 @@ public void addRecord(IndexSegment segment, RecordInfo recordInfo) {
return new RecordLocation(segment, recordInfo.getDocId(), recordInfo.getComparisonValue());
}
});
+
// Update metrics
_serverMetrics.setValueOfPartitionGauge(_tableNameWithType, _partitionId, ServerGauge.UPSERT_PRIMARY_KEYS_COUNT,
_primaryKeyToRecordLocationMap.size());
}
+ /**
+ * Replaces the upsert metadata for the old segment with the new immutable segment.
+ */
+ public void replaceSegment(ImmutableSegment newSegment, IndexSegment oldSegment) {
+ String segmentName = newSegment.getSegmentName();
+ Preconditions.checkArgument(segmentName.equals(oldSegment.getSegmentName()),
+ "Cannot replace segment with different name for table: {}, old segment: {}, new segment: {}",
+ _tableNameWithType, oldSegment.getSegmentName(), segmentName);
+ _logger.info("Replacing upsert metadata for {} segment: {}",
+ oldSegment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName);
+
+ addSegment(newSegment);
+
+ MutableRoaringBitmap validDocIds =
+ oldSegment.getValidDocIds() != null ? oldSegment.getValidDocIds().getMutableRoaringBitmap() : null;
+ if (validDocIds != null && !validDocIds.isEmpty()) {
+ int numDocsNotReplaced = validDocIds.getCardinality();
+ if (_partialUpsertHandler != null) {
+ _logger.error("Got {} primary keys not replaced when replacing segment: {} for partial upsert table. This can "
Review Comment:
```suggestion
_logger.error("{} primary keys were not updated when replacing segment: {} for partial upsert enabled table. This can "
```
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #9095: Enhance upsert metadata handling
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9095:
URL: https://github.com/apache/pinot/pull/9095#discussion_r928155128
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -208,11 +275,82 @@ public void addRecord(IndexSegment segment, RecordInfo recordInfo) {
return new RecordLocation(segment, recordInfo.getDocId(), recordInfo.getComparisonValue());
}
});
+
// Update metrics
_serverMetrics.setValueOfPartitionGauge(_tableNameWithType, _partitionId, ServerGauge.UPSERT_PRIMARY_KEYS_COUNT,
_primaryKeyToRecordLocationMap.size());
}
+ /**
+ * Replaces the upsert metadata for the old segment with the new immutable segment.
+ */
+ public void replaceSegment(ImmutableSegment newSegment, IndexSegment oldSegment) {
+ String segmentName = newSegment.getSegmentName();
+ Preconditions.checkArgument(segmentName.equals(oldSegment.getSegmentName()),
+ "Cannot replace segment with different name for table: {}, old segment: {}, new segment: {}",
+ _tableNameWithType, oldSegment.getSegmentName(), segmentName);
+ _logger.info("Replacing upsert metadata for {} segment: {}",
+ oldSegment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName);
+
+ addSegment(newSegment);
+
+ MutableRoaringBitmap validDocIds =
+ oldSegment.getValidDocIds() != null ? oldSegment.getValidDocIds().getMutableRoaringBitmap() : null;
+ if (validDocIds != null && !validDocIds.isEmpty()) {
+ int numDocsNotReplaced = validDocIds.getCardinality();
+ if (_partialUpsertHandler != null) {
+ _logger.error("Got {} primary keys not replaced when replacing segment: {} for partial upsert table. This can "
+ + "potentially cause inconsistency between replicas", numDocsNotReplaced, segmentName);
+ _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.PARTIAL_UPSERT_ROWS_NOT_REPLACED,
+ numDocsNotReplaced);
+ } else {
+ _logger.info("Got {} primary keys not replaced when replacing segment: {}", numDocsNotReplaced, segmentName);
+ }
+ removeSegment(oldSegment);
+ }
+
+ _logger.info("Finish replacing upsert metadata for segment: {}", segmentName);
+ }
+
+ /**
+ * Removes the upsert metadata for the given segment.
+ */
+ public void removeSegment(IndexSegment segment) {
+ String segmentName = segment.getSegmentName();
+ _logger.info("Removing upsert metadata for segment: {}, primary key count: {}", segmentName,
+ _primaryKeyToRecordLocationMap.size());
+
+ MutableRoaringBitmap validDocIds =
+ segment.getValidDocIds() != null ? segment.getValidDocIds().getMutableRoaringBitmap() : null;
+ if (validDocIds == null || validDocIds.isEmpty()) {
+ _logger.info("Skipping removing upsert metadata for segment without valid docs: {}", segmentName);
+ return;
+ }
+
+ _logger.info("Trying to remove {} primary keys from upsert metadata for segment: {}", validDocIds.getCardinality(),
Review Comment:
nit: This should be a debug log.
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #9095: Enhance upsert metadata handling
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9095:
URL: https://github.com/apache/pinot/pull/9095#issuecomment-1193194039
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9095?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9095](https://codecov.io/gh/apache/pinot/pull/9095?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (06eb4f2) into [master](https://codecov.io/gh/apache/pinot/commit/1211f07ed9602e130da0e83368fcc465519f0fcc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1211f07) will **decrease** coverage by `43.63%`.
> The diff coverage is `0.65%`.
```diff
@@ Coverage Diff @@
## master #9095 +/- ##
=============================================
- Coverage 70.00% 26.37% -43.64%
+ Complexity 4745 42 -4703
=============================================
Files 1838 1826 -12
Lines 97229 96934 -295
Branches 14639 14621 -18
=============================================
- Hits 68069 25570 -42499
- Misses 24434 68872 +44438
+ Partials 4726 2492 -2234
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `26.37% <0.65%> (-0.10%)` | :arrow_down: |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9095?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/common/utils/SegmentUtils.java](https://codecov.io/gh/apache/pinot/pull/9095/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvU2VnbWVudFV0aWxzLmphdmE=) | `41.17% <0.00%> (-35.75%)` | :arrow_down: |
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/9095/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `40.00% <0.00%> (-29.82%)` | :arrow_down: |
| [...ocal/indexsegment/immutable/EmptyIndexSegment.java](https://codecov.io/gh/apache/pinot/pull/9095/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvaW1tdXRhYmxlL0VtcHR5SW5kZXhTZWdtZW50LmphdmE=) | `0.00% <0.00%> (-30.00%)` | :arrow_down: |
| [...l/indexsegment/immutable/ImmutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/9095/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvaW1tdXRhYmxlL0ltbXV0YWJsZVNlZ21lbnRJbXBsLmphdmE=) | `0.00% <0.00%> (-57.32%)` | :arrow_down: |
| [...ocal/indexsegment/mutable/IntermediateSegment.java](https://codecov.io/gh/apache/pinot/pull/9095/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9JbnRlcm1lZGlhdGVTZWdtZW50LmphdmE=) | `0.00% <0.00%> (-72.16%)` | :arrow_down: |
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/9095/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `0.00% <0.00%> (-53.74%)` | :arrow_down: |
| [...ocal/segment/readers/PinotSegmentRecordReader.java](https://codecov.io/gh/apache/pinot/pull/9095/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3JlYWRlcnMvUGlub3RTZWdtZW50UmVjb3JkUmVhZGVyLmphdmE=) | `0.00% <0.00%> (-64.78%)` | :arrow_down: |
| [...t/local/upsert/PartitionUpsertMetadataManager.java](https://codecov.io/gh/apache/pinot/pull/9095/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvUGFydGl0aW9uVXBzZXJ0TWV0YWRhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-75.24%)` | :arrow_down: |
| [...gment/local/upsert/TableUpsertMetadataManager.java](https://codecov.io/gh/apache/pinot/pull/9095/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvVGFibGVVcHNlcnRNZXRhZGF0YU1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-90.91%)` | :arrow_down: |
| [...ava/org/apache/pinot/segment/spi/IndexSegment.java](https://codecov.io/gh/apache/pinot/pull/9095/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL0luZGV4U2VnbWVudC5qYXZh) | `0.00% <ø> (ø)` | |
| ... and [1340 more](https://codecov.io/gh/apache/pinot/pull/9095/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] KKcorps commented on pull request #9095: Enhance upsert metadata handling
Posted by GitBox <gi...@apache.org>.
KKcorps commented on PR #9095:
URL: https://github.com/apache/pinot/pull/9095#issuecomment-1193173538
LGTM!
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9095: Enhance upsert metadata handling
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9095:
URL: https://github.com/apache/pinot/pull/9095#discussion_r928164715
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java:
##########
@@ -208,11 +275,82 @@ public void addRecord(IndexSegment segment, RecordInfo recordInfo) {
return new RecordLocation(segment, recordInfo.getDocId(), recordInfo.getComparisonValue());
}
});
+
// Update metrics
_serverMetrics.setValueOfPartitionGauge(_tableNameWithType, _partitionId, ServerGauge.UPSERT_PRIMARY_KEYS_COUNT,
_primaryKeyToRecordLocationMap.size());
}
+ /**
+ * Replaces the upsert metadata for the old segment with the new immutable segment.
+ */
+ public void replaceSegment(ImmutableSegment newSegment, IndexSegment oldSegment) {
+ String segmentName = newSegment.getSegmentName();
+ Preconditions.checkArgument(segmentName.equals(oldSegment.getSegmentName()),
+ "Cannot replace segment with different name for table: {}, old segment: {}, new segment: {}",
+ _tableNameWithType, oldSegment.getSegmentName(), segmentName);
+ _logger.info("Replacing upsert metadata for {} segment: {}",
+ oldSegment instanceof ImmutableSegment ? "immutable" : "mutable", segmentName);
+
+ addSegment(newSegment);
+
+ MutableRoaringBitmap validDocIds =
+ oldSegment.getValidDocIds() != null ? oldSegment.getValidDocIds().getMutableRoaringBitmap() : null;
+ if (validDocIds != null && !validDocIds.isEmpty()) {
+ int numDocsNotReplaced = validDocIds.getCardinality();
+ if (_partialUpsertHandler != null) {
+ _logger.error("Got {} primary keys not replaced when replacing segment: {} for partial upsert table. This can "
Review Comment:
Yes. Removing primary keys can potentially cause inconsistency for partial upsert table because we won't be able to restore the original primary key
--
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
[GitHub] [pinot] Jackie-Jiang merged pull request #9095: Enhance upsert metadata handling
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged PR #9095:
URL: https://github.com/apache/pinot/pull/9095
--
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