You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "snleee (via GitHub)" <gi...@apache.org> on 2023/03/12 08:04:59 UTC
[GitHub] [pinot] snleee opened a new pull request, #10412: Combine "GET /segments" API & "GET /segments/{tableName}/select"
snleee opened a new pull request, #10412:
URL: https://github.com/apache/pinot/pull/10412
- Add the time filter for "GET /segments" API
- Add deprecated annotation for "GET /segments/{tableName}/select"
--
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 #10412: Combine "GET /segments" API & "GET /segments/{tableName}/select"
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10412:
URL: https://github.com/apache/pinot/pull/10412#issuecomment-1465140861
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10412?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 [#10412](https://codecov.io/gh/apache/pinot/pull/10412?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0812575) into [master](https://codecov.io/gh/apache/pinot/commit/d475712d3136f3841169e66d2ad4da104717618b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d475712) will **decrease** coverage by `39.85%`.
> The diff coverage is `33.33%`.
```diff
@@ Coverage Diff @@
## master #10412 +/- ##
=============================================
- Coverage 64.25% 24.41% -39.85%
+ Complexity 6059 49 -6010
=============================================
Files 1997 2026 +29
Lines 108768 110473 +1705
Branches 16620 16824 +204
=============================================
- Hits 69888 26971 -42917
- Misses 33816 80689 +46873
+ Partials 5064 2813 -2251
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.41% <33.33%> (?)` | |
| 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/10412?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...spi/utils/builder/ControllerRequestURLBuilder.java](https://codecov.io/gh/apache/pinot/pull/10412?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvYnVpbGRlci9Db250cm9sbGVyUmVxdWVzdFVSTEJ1aWxkZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ler/api/resources/PinotSegmentRestletResource.java](https://codecov.io/gh/apache/pinot/pull/10412?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90U2VnbWVudFJlc3RsZXRSZXNvdXJjZS5qYXZh) | `28.84% <37.50%> (+18.87%)` | :arrow_up: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/10412?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `39.52% <71.42%> (-23.08%)` | :arrow_down: |
| [...e/pinot/common/utils/FileUploadDownloadClient.java](https://codecov.io/gh/apache/pinot/pull/10412?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvRmlsZVVwbG9hZERvd25sb2FkQ2xpZW50LmphdmE=) | `49.79% <100.00%> (+35.50%)` | :arrow_up: |
| [...ot/plugin/minion/tasks/SegmentConversionUtils.java](https://codecov.io/gh/apache/pinot/pull/10412?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvU2VnbWVudENvbnZlcnNpb25VdGlscy5qYXZh) | `76.71% <100.00%> (+62.82%)` | :arrow_up: |
... and [1661 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10412/indirect-changes?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 #10412: Combine "GET /segments" API & "GET /segments/{tableName}/select"
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #10412:
URL: https://github.com/apache/pinot/pull/10412#discussion_r1154016890
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -739,36 +739,27 @@ public String getActualTableName(String tableName) {
* @return List of segment names
*/
public List<String> getSegmentsFor(String tableNameWithType, boolean shouldExcludeReplacedSegments) {
- IdealState idealState = getTableIdealState(tableNameWithType);
- Preconditions.checkState(idealState != null, "Failed to find ideal state for table: %s", tableNameWithType);
- List<String> segments = new ArrayList<>(idealState.getPartitionSet());
- return shouldExcludeReplacedSegments ? excludeReplacedSegments(tableNameWithType, segments) : segments;
- }
-
- /**
- * Returns the segments for the given table from the property store. This API is useful to track the orphan segments
- * that are removed from the ideal state but not the property store.
- */
- public List<String> getSegmentsFromPropertyStore(String tableNameWithType) {
- return ZKMetadataProvider.getSegments(_propertyStore, tableNameWithType);
+ return getSegmentsFor(tableNameWithType, shouldExcludeReplacedSegments, Long.MIN_VALUE, Long.MAX_VALUE, false);
}
/**
- * Returns the segments for the given table based on the start and end timestamp from the ideal state.
+ * Returns the segments for the given table from the ideal state.
*
- * @param tableNameWithType Table name with type suffix
+ * @param tableNameWithType Table name with type suffix
+ * @param shouldExcludeReplacedSegments whether to return the list of segments that doesn't contain replaced segments.
* @param startTimestamp start timestamp in milliseconds (inclusive)
* @param endTimestamp end timestamp in milliseconds (exclusive)
* @param excludeOverlapping whether to exclude the segments overlapping with the timestamps
+ * @return List of segment names
*/
- public List<String> getSegmentsForTableWithTimestamps(String tableNameWithType, long startTimestamp,
- long endTimestamp, boolean excludeOverlapping) {
+ public List<String> getSegmentsFor(String tableNameWithType, boolean shouldExcludeReplacedSegments,
+ long startTimestamp, long endTimestamp, boolean excludeOverlapping) {
IdealState idealState = getTableIdealState(tableNameWithType);
Preconditions.checkState(idealState != null, "Failed to find ideal state for table: %s", tableNameWithType);
- Set<String> segments = idealState.getPartitionSet();
- // If no start and end timestamp specified, just select all the segments.
+ List<String> segments = new ArrayList<>(idealState.getPartitionSet());
if (startTimestamp == Long.MIN_VALUE && endTimestamp == Long.MAX_VALUE) {
- return excludeReplacedSegments(tableNameWithType, new ArrayList<>(segments));
+ return shouldExcludeReplacedSegments ? excludeReplacedSegments(tableNameWithType, new ArrayList<>(segments))
Review Comment:
good catch. fixed.
--
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 #10412: Combine "GET /segments" API & "GET /segments/{tableName}/select"
Posted by "jtao15 (via GitHub)" <gi...@apache.org>.
jtao15 commented on code in PR #10412:
URL: https://github.com/apache/pinot/pull/10412#discussion_r1133337390
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -739,36 +739,27 @@ public String getActualTableName(String tableName) {
* @return List of segment names
*/
public List<String> getSegmentsFor(String tableNameWithType, boolean shouldExcludeReplacedSegments) {
- IdealState idealState = getTableIdealState(tableNameWithType);
- Preconditions.checkState(idealState != null, "Failed to find ideal state for table: %s", tableNameWithType);
- List<String> segments = new ArrayList<>(idealState.getPartitionSet());
- return shouldExcludeReplacedSegments ? excludeReplacedSegments(tableNameWithType, segments) : segments;
- }
-
- /**
- * Returns the segments for the given table from the property store. This API is useful to track the orphan segments
- * that are removed from the ideal state but not the property store.
- */
- public List<String> getSegmentsFromPropertyStore(String tableNameWithType) {
- return ZKMetadataProvider.getSegments(_propertyStore, tableNameWithType);
+ return getSegmentsFor(tableNameWithType, shouldExcludeReplacedSegments, Long.MIN_VALUE, Long.MAX_VALUE, false);
}
/**
- * Returns the segments for the given table based on the start and end timestamp from the ideal state.
+ * Returns the segments for the given table from the ideal state.
*
- * @param tableNameWithType Table name with type suffix
+ * @param tableNameWithType Table name with type suffix
+ * @param shouldExcludeReplacedSegments whether to return the list of segments that doesn't contain replaced segments.
* @param startTimestamp start timestamp in milliseconds (inclusive)
* @param endTimestamp end timestamp in milliseconds (exclusive)
* @param excludeOverlapping whether to exclude the segments overlapping with the timestamps
+ * @return List of segment names
*/
- public List<String> getSegmentsForTableWithTimestamps(String tableNameWithType, long startTimestamp,
- long endTimestamp, boolean excludeOverlapping) {
+ public List<String> getSegmentsFor(String tableNameWithType, boolean shouldExcludeReplacedSegments,
+ long startTimestamp, long endTimestamp, boolean excludeOverlapping) {
IdealState idealState = getTableIdealState(tableNameWithType);
Preconditions.checkState(idealState != null, "Failed to find ideal state for table: %s", tableNameWithType);
- Set<String> segments = idealState.getPartitionSet();
- // If no start and end timestamp specified, just select all the segments.
+ List<String> segments = new ArrayList<>(idealState.getPartitionSet());
if (startTimestamp == Long.MIN_VALUE && endTimestamp == Long.MAX_VALUE) {
- return excludeReplacedSegments(tableNameWithType, new ArrayList<>(segments));
+ return shouldExcludeReplacedSegments ? excludeReplacedSegments(tableNameWithType, new ArrayList<>(segments))
Review Comment:
(nit) we don't need to create a new `ArrayList`? Use `excludeReplacedSegments(tableNameWithType, segments)` instead? Same for `L774`.
--
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 #10412: Combine "GET /segments" API & "GET /segments/{tableName}/select"
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #10412:
URL: https://github.com/apache/pinot/pull/10412#discussion_r1154015133
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java:
##########
@@ -379,26 +379,45 @@ public String forDeleteTableWithType(String tableName, String tableType) {
}
public String forSegmentListAPI(String tableName) {
- return forSegmentListAPI(tableName, null, false);
+ return forSegmentListAPI(tableName, null, false, Long.MIN_VALUE, Long.MAX_VALUE, false);
}
public String forSegmentListAPI(String tableName, String tableType) {
- return forSegmentListAPI(tableName, tableType, false);
+ return forSegmentListAPI(tableName, tableType, false, Long.MIN_VALUE, Long.MAX_VALUE, false);
}
- public String forSegmentListAPI(String tableName, @Nullable String tableType, boolean excludeReplacedSegments) {
+ public String forSegmentListAPI(String tableName, String tableType, boolean excludeReplacedSegments) {
+ return forSegmentListAPI(tableName, tableType, excludeReplacedSegments, Long.MIN_VALUE, Long.MAX_VALUE, false);
+ }
+
+ public String forSegmentListAPI(String tableName, @Nullable String tableType, boolean excludeReplacedSegments,
+ long startTimestamp, long endTimestamp, boolean excludeOverlapping) {
String url = StringUtil.join("/", _baseUrl, "segments", tableName);
+ String urlParameter = "";
Review Comment:
fixed
--
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 #10412: Combine "GET /segments" API & "GET /segments/{tableName}/select"
Posted by "jackjlli (via GitHub)" <gi...@apache.org>.
jackjlli commented on code in PR #10412:
URL: https://github.com/apache/pinot/pull/10412#discussion_r1133315178
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java:
##########
@@ -379,26 +379,45 @@ public String forDeleteTableWithType(String tableName, String tableType) {
}
public String forSegmentListAPI(String tableName) {
- return forSegmentListAPI(tableName, null, false);
+ return forSegmentListAPI(tableName, null, false, Long.MIN_VALUE, Long.MAX_VALUE, false);
}
public String forSegmentListAPI(String tableName, String tableType) {
- return forSegmentListAPI(tableName, tableType, false);
+ return forSegmentListAPI(tableName, tableType, false, Long.MIN_VALUE, Long.MAX_VALUE, false);
}
- public String forSegmentListAPI(String tableName, @Nullable String tableType, boolean excludeReplacedSegments) {
+ public String forSegmentListAPI(String tableName, String tableType, boolean excludeReplacedSegments) {
+ return forSegmentListAPI(tableName, tableType, excludeReplacedSegments, Long.MIN_VALUE, Long.MAX_VALUE, false);
+ }
+
+ public String forSegmentListAPI(String tableName, @Nullable String tableType, boolean excludeReplacedSegments,
+ long startTimestamp, long endTimestamp, boolean excludeOverlapping) {
String url = StringUtil.join("/", _baseUrl, "segments", tableName);
+ String urlParameter = "";
Review Comment:
nit: replace it with `StringBuilder` approach?
--
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] zhtaoxiang commented on a diff in pull request #10412: Combine "GET /segments" API & "GET /segments/{tableName}/select"
Posted by "zhtaoxiang (via GitHub)" <gi...@apache.org>.
zhtaoxiang commented on code in PR #10412:
URL: https://github.com/apache/pinot/pull/10412#discussion_r1134731059
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -197,16 +197,28 @@ public List<Map<TableType, List<String>>> getSegments(
@ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
@ApiParam(value = "Whether to exclude replaced segments in the response, which have been replaced"
+ " specified in the segment lineage entries and cannot be queried from the table")
- @QueryParam("excludeReplacedSegments") String excludeReplacedSegments) {
- List<String> tableNamesWithType = ResourceUtils
- .getExistingTableNamesWithType(_pinotHelixResourceManager, tableName, Constants.validateTableType(tableTypeStr),
- LOGGER);
+ @QueryParam("excludeReplacedSegments") String excludeReplacedSegments,
+ @ApiParam(value = "Start timestamp (inclusive)") @QueryParam("startTimestamp") @DefaultValue("")
+ String startTimestampStr,
+ @ApiParam(value = "End timestamp (exclusive)") @QueryParam("endTimestamp") @DefaultValue("")
+ String endTimestampStr,
+ @ApiParam(value = "Whether to exclude the segments overlapping with the timestamps, false by default")
+ @QueryParam("excludeOverlapping") @DefaultValue("false") boolean excludeOverlapping) {
+ long startTimestamp = Strings.isNullOrEmpty(startTimestampStr) ? Long.MIN_VALUE : Long.parseLong(startTimestampStr);
+ long endTimestamp = Strings.isNullOrEmpty(endTimestampStr) ? Long.MAX_VALUE : Long.parseLong(endTimestampStr);
Review Comment:
maybe we want to catch `NumberFormatException` and handle it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] snleee commented on a diff in pull request #10412: Combine "GET /segments" API & "GET /segments/{tableName}/select"
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #10412:
URL: https://github.com/apache/pinot/pull/10412#discussion_r1154019467
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -197,16 +197,28 @@ public List<Map<TableType, List<String>>> getSegments(
@ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
@ApiParam(value = "Whether to exclude replaced segments in the response, which have been replaced"
+ " specified in the segment lineage entries and cannot be queried from the table")
- @QueryParam("excludeReplacedSegments") String excludeReplacedSegments) {
- List<String> tableNamesWithType = ResourceUtils
- .getExistingTableNamesWithType(_pinotHelixResourceManager, tableName, Constants.validateTableType(tableTypeStr),
- LOGGER);
+ @QueryParam("excludeReplacedSegments") String excludeReplacedSegments,
+ @ApiParam(value = "Start timestamp (inclusive)") @QueryParam("startTimestamp") @DefaultValue("")
+ String startTimestampStr,
+ @ApiParam(value = "End timestamp (exclusive)") @QueryParam("endTimestamp") @DefaultValue("")
+ String endTimestampStr,
+ @ApiParam(value = "Whether to exclude the segments overlapping with the timestamps, false by default")
+ @QueryParam("excludeOverlapping") @DefaultValue("false") boolean excludeOverlapping) {
+ long startTimestamp = Strings.isNullOrEmpty(startTimestampStr) ? Long.MIN_VALUE : Long.parseLong(startTimestampStr);
+ long endTimestamp = Strings.isNullOrEmpty(endTimestampStr) ? Long.MAX_VALUE : Long.parseLong(endTimestampStr);
Review Comment:
added
--
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 merged pull request #10412: Combine "GET /segments" API & "GET /segments/{tableName}/select"
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee merged PR #10412:
URL: https://github.com/apache/pinot/pull/10412
--
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