You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yuanbenson (via GitHub)" <gi...@apache.org> on 2023/02/18 01:01:25 UTC
[GitHub] [pinot] yuanbenson opened a new pull request, #10303: Add excludeReplacedSegments param into table metadata API
yuanbenson opened a new pull request, #10303:
URL: https://github.com/apache/pinot/pull/10303
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
2. `bugfix`
3. `performance`
4. `ui`
5. `backward-incompat`
6. `release-notes` (**)
2. Remove these instructions before publishing the PR.
(*) Other labels to consider:
- `testing`
- `dependencies`
- `docker`
- `kubernetes`
- `observability`
- `security`
- `code-style`
- `extension-point`
- `refactor`
- `cleanup`
(**) Use `release-notes` label for scenarios like:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
--
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] yuanbenson commented on a diff in pull request #10303: Add excludeReplacedSegments param into table metadata API
Posted by "yuanbenson (via GitHub)" <gi...@apache.org>.
yuanbenson commented on code in PR #10303:
URL: https://github.com/apache/pinot/pull/10303#discussion_r1113523394
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MergeRollupMinionClusterIntegrationTest.java:
##########
@@ -626,6 +625,23 @@ public void testOfflineTableSingleLevelRollup()
assertTrue(MetricValueUtils.gaugeExists(_controllerStarter.getControllerMetrics(),
"mergeRollupTaskDelayInNumBuckets.myTable2_OFFLINE.150days"));
+
+ JsonNode metadataResponseWithReplacedSegments = JsonUtils.stringToJsonNode(sendGetRequest(
+ _controllerBaseApiUrl + "/tables/" + SINGLE_LEVEL_ROLLUP_TEST_TABLE
+ + "/metadata?excludeReplacedSegments=false"));
+ JsonNode metadataResponseWithoutReplacedSegments = JsonUtils.stringToJsonNode(sendGetRequest(
+ _controllerBaseApiUrl + "/tables/" + SINGLE_LEVEL_ROLLUP_TEST_TABLE
+ + "/metadata?excludeReplacedSegments=true"));
+
+ String tableNameWithType = TableNameBuilder.OFFLINE.tableNameWithType(SINGLE_LEVEL_ROLLUP_TEST_TABLE);
+ assertEquals(_helixResourceManager.getSegmentsFor(tableNameWithType, true).size(),
+ metadataResponseWithoutReplacedSegments.get("numSegments").asInt());
+ assertEquals(_helixResourceManager.getSegmentsFor(tableNameWithType, false).size(),
+ metadataResponseWithReplacedSegments.get("numSegments").asInt());
+
+ int numRowsFromMetadataWithoutReplacedSegments = metadataResponseWithoutReplacedSegments.get("numRows").asInt();
+ assertNotEquals(metadataResponseWithReplacedSegments, metadataResponseWithoutReplacedSegments);
+ assertEquals(numRowsAfterMergeTasks, numRowsFromMetadataWithoutReplacedSegments);
Review Comment:
There is already existing test https://github.com/apache/pinot/blob/master/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java#L2621 for /metadata endpoint. However, I had trouble modifying the code to allow additional segments upload in which to trigger the consistent data push protocol with and thought `MergeRollupMinionClusterIntegrationTest` is the most convenient place to test this new param.
--
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] GSharayu commented on a diff in pull request #10303: Add excludeReplacedSegments param into table metadata API
Posted by "GSharayu (via GitHub)" <gi...@apache.org>.
GSharayu commented on code in PR #10303:
URL: https://github.com/apache/pinot/pull/10303#discussion_r1113512462
##########
pinot-common/src/main/java/org/apache/pinot/common/lineage/SegmentLineageUtils.java:
##########
@@ -53,4 +54,19 @@ public static void filterSegmentsBasedOnLineageInPlace(Set<String> segments, Seg
}
}
}
+
+ public static Set<String> getDuplicateSegmentsToFilterOut(SegmentLineage segmentLineage) {
Review Comment:
minor nit: Do we want to rename this to replacedSegments? duplicateSegments might not justify the API purpose. wdyt?
--
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] yuanbenson commented on a diff in pull request #10303: Add excludeReplacedSegments param into table metadata API
Posted by "yuanbenson (via GitHub)" <gi...@apache.org>.
yuanbenson commented on code in PR #10303:
URL: https://github.com/apache/pinot/pull/10303#discussion_r1113518075
##########
pinot-common/src/main/java/org/apache/pinot/common/lineage/SegmentLineageUtils.java:
##########
@@ -53,4 +54,19 @@ public static void filterSegmentsBasedOnLineageInPlace(Set<String> segments, Seg
}
}
}
+
+ public static Set<String> getDuplicateSegmentsToFilterOut(SegmentLineage segmentLineage) {
Review Comment:
Yes, good point. Replaced segments don't necessarily mean duplicates, made this over-simplification while implementing by mistake :)
--
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] jackjlli commented on a diff in pull request #10303: Add excludeReplacedSegments param into table metadata API
Posted by "jackjlli (via GitHub)" <gi...@apache.org>.
jackjlli commented on code in PR #10303:
URL: https://github.com/apache/pinot/pull/10303#discussion_r1115105573
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -808,7 +808,9 @@ public String getTableAggregateMetadata(
@ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
@ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
@ApiParam(value = "Columns name", allowMultiple = true) @QueryParam("columns") @DefaultValue("")
- List<String> columns) {
+ List<String> columns,
+ @ApiParam(value = "Whether to exclude replaced segments in the response") @QueryParam("excludeReplacedSegments")
+ @DefaultValue("false") String excludeReplacedSegments) {
Review Comment:
+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
[GitHub] [pinot] jtao15 commented on a diff in pull request #10303: Add excludeReplacedSegments param into table metadata API
Posted by "jtao15 (via GitHub)" <gi...@apache.org>.
jtao15 commented on code in PR #10303:
URL: https://github.com/apache/pinot/pull/10303#discussion_r1115144518
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -265,13 +269,31 @@ public boolean isSegmentDeletedRecently(String segmentName) {
@Override
public List<SegmentDataManager> acquireAllSegments() {
+ return acquireAllSegments(false);
+ }
+
+ @Override
+ public List<SegmentDataManager> acquireAllSegments(boolean excludeReplacedSegments) {
List<SegmentDataManager> segmentDataManagers = new ArrayList<>();
for (SegmentDataManager segmentDataManager : _segmentDataManagerMap.values()) {
if (segmentDataManager.increaseReferenceCount()) {
segmentDataManagers.add(segmentDataManager);
}
}
- return segmentDataManagers;
+ // Fetch the segment lineage metadata, and conditionally filter out replaced segments based on segment lineage.
+ SegmentLineage segmentLineage = SegmentLineageAccessHelper.getSegmentLineage(_propertyStore, _tableNameWithType);
Review Comment:
+1 on this, it's also possible that different servers get different versions of the lineage metadata, and controllers return incorrect results due to 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
[GitHub] [pinot] sajjad-moradi commented on a diff in pull request #10303: Add excludeReplacedSegments param into table metadata API
Posted by "sajjad-moradi (via GitHub)" <gi...@apache.org>.
sajjad-moradi commented on code in PR #10303:
URL: https://github.com/apache/pinot/pull/10303#discussion_r1113889192
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -265,13 +269,31 @@ public boolean isSegmentDeletedRecently(String segmentName) {
@Override
public List<SegmentDataManager> acquireAllSegments() {
+ return acquireAllSegments(false);
+ }
+
+ @Override
+ public List<SegmentDataManager> acquireAllSegments(boolean excludeReplacedSegments) {
List<SegmentDataManager> segmentDataManagers = new ArrayList<>();
for (SegmentDataManager segmentDataManager : _segmentDataManagerMap.values()) {
if (segmentDataManager.increaseReferenceCount()) {
segmentDataManagers.add(segmentDataManager);
}
}
- return segmentDataManagers;
+ // Fetch the segment lineage metadata, and conditionally filter out replaced segments based on segment lineage.
+ SegmentLineage segmentLineage = SegmentLineageAccessHelper.getSegmentLineage(_propertyStore, _tableNameWithType);
+ if (segmentLineage == null || !excludeReplacedSegments) {
+ return segmentDataManagers;
+ } else {
+ Set<String> replacedSegments = SegmentLineageUtils.getReplacedSegments(segmentLineage);
+ List<SegmentDataManager> filteredSegmentDataManagers = new ArrayList<>();
+ for (SegmentDataManager segmentDataManager : segmentDataManagers) {
+ if (!replacedSegments.contains(segmentDataManager.getSegmentName())) {
+ filteredSegmentDataManagers.add(segmentDataManager);
+ }
+ }
+ return filteredSegmentDataManagers;
+ }
Review Comment:
There are a few problems here:
1. If the input flag excludeReplacedSegments is false, we still get segment lineage from ZK. This is not desirable as it adds unnecessary calls to ZK.
2. Here all segmentDataManagers are acquired, but in the return list, some of them are filtered out. In TablesResource, at the end of the endpoint, all of segmentDataManagers must be released, otherwise the memory resources for the filtered out segments will not be cleaned properly.
3. Most importantly, the functionality added here doesn't belong to this class. It's something that's being used only in TablesResources. I suggest moving the new code to the calling endpoint and not change the interface.
--
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] yuanbenson commented on a diff in pull request #10303: Add excludeReplacedSegments param into table metadata API
Posted by "yuanbenson (via GitHub)" <gi...@apache.org>.
yuanbenson commented on code in PR #10303:
URL: https://github.com/apache/pinot/pull/10303#discussion_r1113574438
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/data/manager/TableDataManager.java:
##########
@@ -139,6 +139,14 @@ void addOrReplaceSegment(String segmentName, IndexLoadingConfig indexLoadingConf
*/
List<SegmentDataManager> acquireAllSegments();
+ /**
+ * Acquires all segments of the table.
+ * <p>It is the caller's responsibility to return the segments by calling {@link #releaseSegment(SegmentDataManager)}.
+ *
+ * @return List of segment data managers
+ */
Review Comment:
Good catch, addressed.
--
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] GSharayu commented on a diff in pull request #10303: Add excludeReplacedSegments param into table metadata API
Posted by "GSharayu (via GitHub)" <gi...@apache.org>.
GSharayu commented on code in PR #10303:
URL: https://github.com/apache/pinot/pull/10303#discussion_r1113429711
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MergeRollupMinionClusterIntegrationTest.java:
##########
@@ -626,6 +625,23 @@ public void testOfflineTableSingleLevelRollup()
assertTrue(MetricValueUtils.gaugeExists(_controllerStarter.getControllerMetrics(),
"mergeRollupTaskDelayInNumBuckets.myTable2_OFFLINE.150days"));
+
+ JsonNode metadataResponseWithReplacedSegments = JsonUtils.stringToJsonNode(sendGetRequest(
+ _controllerBaseApiUrl + "/tables/" + SINGLE_LEVEL_ROLLUP_TEST_TABLE
+ + "/metadata?excludeReplacedSegments=false"));
+ JsonNode metadataResponseWithoutReplacedSegments = JsonUtils.stringToJsonNode(sendGetRequest(
+ _controllerBaseApiUrl + "/tables/" + SINGLE_LEVEL_ROLLUP_TEST_TABLE
+ + "/metadata?excludeReplacedSegments=true"));
+
+ String tableNameWithType = TableNameBuilder.OFFLINE.tableNameWithType(SINGLE_LEVEL_ROLLUP_TEST_TABLE);
+ assertEquals(_helixResourceManager.getSegmentsFor(tableNameWithType, true).size(),
+ metadataResponseWithoutReplacedSegments.get("numSegments").asInt());
+ assertEquals(_helixResourceManager.getSegmentsFor(tableNameWithType, false).size(),
+ metadataResponseWithReplacedSegments.get("numSegments").asInt());
+
+ int numRowsFromMetadataWithoutReplacedSegments = metadataResponseWithoutReplacedSegments.get("numRows").asInt();
+ assertNotEquals(metadataResponseWithReplacedSegments, metadataResponseWithoutReplacedSegments);
+ assertEquals(numRowsAfterMergeTasks, numRowsFromMetadataWithoutReplacedSegments);
Review Comment:
Apart from consistent push, If possible, we should also add a unit test which verifies /metadata endpoint independently
--
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] GSharayu commented on a diff in pull request #10303: Add excludeReplacedSegments param into table metadata API
Posted by "GSharayu (via GitHub)" <gi...@apache.org>.
GSharayu commented on code in PR #10303:
URL: https://github.com/apache/pinot/pull/10303#discussion_r1113537124
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/data/manager/TableDataManager.java:
##########
@@ -139,6 +139,14 @@ void addOrReplaceSegment(String segmentName, IndexLoadingConfig indexLoadingConf
*/
List<SegmentDataManager> acquireAllSegments();
+ /**
+ * Acquires all segments of the table.
+ * <p>It is the caller's responsibility to return the segments by calling {@link #releaseSegment(SegmentDataManager)}.
+ *
+ * @return List of segment data managers
+ */
Review Comment:
minor again, want to introduce the param in doc above?
--
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 #10303: Add excludeReplacedSegments param into table metadata API
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10303:
URL: https://github.com/apache/pinot/pull/10303#issuecomment-1435467110
# [Codecov](https://codecov.io/gh/apache/pinot/pull/10303?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 [#10303](https://codecov.io/gh/apache/pinot/pull/10303?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (de649b0) into [master](https://codecov.io/gh/apache/pinot/commit/ba8aad7ad0c2e13a2e357ab68ccb817c375ae190?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ba8aad7) will **decrease** coverage by `56.72%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10303 +/- ##
=============================================
- Coverage 70.42% 13.70% -56.72%
+ Complexity 5119 182 -4937
=============================================
Files 2016 1961 -55
Lines 109654 107209 -2445
Branches 16683 16397 -286
=============================================
- Hits 77221 14697 -62524
- Misses 27010 91344 +64334
+ Partials 5423 1168 -4255
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.70% <0.00%> (+<0.01%)` | :arrow_up: |
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/10303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ache/pinot/common/lineage/SegmentLineageUtils.java](https://codecov.io/gh/apache/pinot/pull/10303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbGluZWFnZS9TZWdtZW50TGluZWFnZVV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...oller/api/resources/PinotTableRestletResource.java](https://codecov.io/gh/apache/pinot/pull/10303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90VGFibGVSZXN0bGV0UmVzb3VyY2UuamF2YQ==) | `52.08% <0.00%> (-3.27%)` | :arrow_down: |
| [...t/controller/util/ServerSegmentMetadataReader.java](https://codecov.io/gh/apache/pinot/pull/10303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci91dGlsL1NlcnZlclNlZ21lbnRNZXRhZGF0YVJlYWRlci5qYXZh) | `30.00% <0.00%> (-54.91%)` | :arrow_down: |
| [...che/pinot/controller/util/TableMetadataReader.java](https://codecov.io/gh/apache/pinot/pull/10303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci91dGlsL1RhYmxlTWV0YWRhdGFSZWFkZXIuamF2YQ==) | `0.00% <0.00%> (-63.42%)` | :arrow_down: |
| [.../pinot/core/data/manager/BaseTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/10303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvQmFzZVRhYmxlRGF0YU1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-75.60%)` | :arrow_down: |
| [...t/segment/local/data/manager/TableDataManager.java](https://codecov.io/gh/apache/pinot/pull/10303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9kYXRhL21hbmFnZXIvVGFibGVEYXRhTWFuYWdlci5qYXZh) | `0.00% <ø> (ø)` | |
| [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/10303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/10303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/10303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/common/CustomObject.java](https://codecov.io/gh/apache/pinot/pull/10303?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vQ3VzdG9tT2JqZWN0LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1591 more](https://codecov.io/gh/apache/pinot/pull/10303?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
: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=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] snleee commented on a diff in pull request #10303: Add excludeReplacedSegments param into table metadata API
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #10303:
URL: https://github.com/apache/pinot/pull/10303#discussion_r1113886403
##########
pinot-common/src/main/java/org/apache/pinot/common/lineage/SegmentLineageUtils.java:
##########
@@ -53,4 +54,19 @@ public static void filterSegmentsBasedOnLineageInPlace(Set<String> segments, Seg
}
}
}
+
+ public static Set<String> getReplacedSegments(SegmentLineage segmentLineage) {
Review Comment:
can we modify the `filterSegmentsBasedOnLineageInPlace` to use `getReplacedSegments`? In that way, we can keep the core logic in a single place.
```
Set<String> replacedSegments = getReplacedSegments()
segments.removeAll(replacedSegments)
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -808,7 +808,9 @@ public String getTableAggregateMetadata(
@ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
@ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
@ApiParam(value = "Columns name", allowMultiple = true) @QueryParam("columns") @DefaultValue("")
- List<String> columns) {
+ List<String> columns,
+ @ApiParam(value = "Whether to exclude replaced segments in the response") @QueryParam("excludeReplacedSegments")
+ @DefaultValue("false") String excludeReplacedSegments) {
Review Comment:
Can you check other APIs to see if we can directly read the parameter as `boolean`? I think that we supported that.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -265,13 +269,31 @@ public boolean isSegmentDeletedRecently(String segmentName) {
@Override
public List<SegmentDataManager> acquireAllSegments() {
+ return acquireAllSegments(false);
+ }
+
+ @Override
+ public List<SegmentDataManager> acquireAllSegments(boolean excludeReplacedSegments) {
List<SegmentDataManager> segmentDataManagers = new ArrayList<>();
for (SegmentDataManager segmentDataManager : _segmentDataManagerMap.values()) {
if (segmentDataManager.increaseReferenceCount()) {
segmentDataManagers.add(segmentDataManager);
}
}
- return segmentDataManagers;
+ // Fetch the segment lineage metadata, and conditionally filter out replaced segments based on segment lineage.
+ SegmentLineage segmentLineage = SegmentLineageAccessHelper.getSegmentLineage(_propertyStore, _tableNameWithType);
Review Comment:
Can we filter out the segments based on the lineage during the aggregation phase in the controller api logic? I think that your code change will become much much simpler.
Currently, you are pushing down the filtering all the way to the server side and we are even accessing the lineage from the server side. I don't think that it's required based on the motivation from https://github.com/apache/pinot/issues/10244.
--
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