[GitHub] [pinot] jtao15 commented on a diff in pull request #10412: Combine "GET /segments" API & "GET /segments/{tableName}/select"

["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