Copilot commented on code in PR #16940:
URL: https://github.com/apache/pinot/pull/16940#discussion_r2395705986
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/ServerSegmentMetadataReader.java:
##########
@@ -430,26 +430,48 @@ public Map<String, TableStaleSegmentResponse>
getStaleSegmentsFromServer(
return serverResponses;
}
- private String generateAggregateSegmentMetadataServerURL(String
tableNameWithType, List<String> columns,
+ private String generateAggregateSegmentMetadataServerURL(String
tableNameWithType, @Nullable List<String> columns,
String endpoint) {
tableNameWithType = URLEncoder.encode(tableNameWithType,
StandardCharsets.UTF_8);
- String paramsStr = generateParam(COLUMNS_KEY, columns);
- return String.format("%s/tables/%s/metadata?%s", endpoint,
tableNameWithType, paramsStr);
+ if (CollectionUtils.isEmpty(columns)) {
+ return String.format("%s/tables/%s/metadata", endpoint,
tableNameWithType);
+ } else {
+ return String.format("%s/tables/%s/metadata?%s", endpoint,
tableNameWithType,
+ generateParam(COLUMNS_KEY, columns));
+ }
}
public String generateSegmentMetadataServerURL(String tableNameWithType,
String segmentName,
@Nullable List<String> columns, String endpoint) {
tableNameWithType = URLEncoder.encode(tableNameWithType,
StandardCharsets.UTF_8);
segmentName = URLEncoder.encode(segmentName, StandardCharsets.UTF_8);
- String paramsStr = generateParam(COLUMNS_KEY, columns);
- return String.format("%s/tables/%s/segments/%s/metadata?%s", endpoint,
tableNameWithType, segmentName, paramsStr);
+ if (CollectionUtils.isEmpty(columns)) {
+ return String.format("%s/tables/%s/segments/%s/metadata", endpoint,
tableNameWithType, segmentName);
+ } else {
+ return String.format("%s/tables/%s/segments/%s/metadata?%s", endpoint,
tableNameWithType, segmentName,
+ generateParam(COLUMNS_KEY, columns));
+ }
}
public String generateTableMetadataServerURL(String tableNameWithType,
@Nullable List<String> columns,
- @Nullable List<String> segmentsToInclude, String endpoint) {
+ @Nullable List<String> segments, String endpoint) {
tableNameWithType = URLEncoder.encode(tableNameWithType,
StandardCharsets.UTF_8);
- String paramsStr = generateParam(COLUMNS_KEY, columns) + "&" +
generateParam(SEGMENTS_KEY, segmentsToInclude);
- return String.format("%s/tables/%s/segments/metadata?%s", endpoint,
tableNameWithType, paramsStr);
+ if (CollectionUtils.isEmpty(columns)) {
+ if (CollectionUtils.isEmpty(segments)) {
+ return String.format("%s/tables/%s/metadata", endpoint,
tableNameWithType);
+ } else {
+ return String.format("%s/tables/%s/segments/metadata?%s", endpoint,
tableNameWithType,
+ generateParam(SEGMENTS_KEY, segments));
+ }
+ } else {
+ if (CollectionUtils.isEmpty(segments)) {
+ return String.format("%s/tables/%s/metadata?%s", endpoint,
tableNameWithType,
+ generateParam(COLUMNS_KEY, columns));
Review Comment:
Inconsistent API endpoints: when segments is empty, the method returns
`/tables/{table}/metadata` but when segments is not empty, it returns
`/tables/{table}/segments/metadata`. Based on the method name
`generateTableMetadataServerURL` and the parameter name `segments`, it should
consistently use the `/segments/metadata` endpoint when segments are provided.
```suggestion
if (CollectionUtils.isEmpty(segments)) {
if (CollectionUtils.isEmpty(columns)) {
return String.format("%s/tables/%s/metadata", endpoint,
tableNameWithType);
} else {
return String.format("%s/tables/%s/metadata?%s", endpoint,
tableNameWithType,
generateParam(COLUMNS_KEY, columns));
}
} else {
// segments is not empty, always use /segments/metadata endpoint
if (CollectionUtils.isEmpty(columns)) {
return String.format("%s/tables/%s/segments/metadata?%s", endpoint,
tableNameWithType,
generateParam(SEGMENTS_KEY, segments));
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/ServerSegmentMetadataReader.java:
##########
@@ -430,26 +430,48 @@ public Map<String, TableStaleSegmentResponse>
getStaleSegmentsFromServer(
return serverResponses;
}
- private String generateAggregateSegmentMetadataServerURL(String
tableNameWithType, List<String> columns,
+ private String generateAggregateSegmentMetadataServerURL(String
tableNameWithType, @Nullable List<String> columns,
String endpoint) {
tableNameWithType = URLEncoder.encode(tableNameWithType,
StandardCharsets.UTF_8);
- String paramsStr = generateParam(COLUMNS_KEY, columns);
- return String.format("%s/tables/%s/metadata?%s", endpoint,
tableNameWithType, paramsStr);
+ if (CollectionUtils.isEmpty(columns)) {
+ return String.format("%s/tables/%s/metadata", endpoint,
tableNameWithType);
+ } else {
+ return String.format("%s/tables/%s/metadata?%s", endpoint,
tableNameWithType,
+ generateParam(COLUMNS_KEY, columns));
+ }
}
public String generateSegmentMetadataServerURL(String tableNameWithType,
String segmentName,
@Nullable List<String> columns, String endpoint) {
tableNameWithType = URLEncoder.encode(tableNameWithType,
StandardCharsets.UTF_8);
segmentName = URLEncoder.encode(segmentName, StandardCharsets.UTF_8);
- String paramsStr = generateParam(COLUMNS_KEY, columns);
- return String.format("%s/tables/%s/segments/%s/metadata?%s", endpoint,
tableNameWithType, segmentName, paramsStr);
+ if (CollectionUtils.isEmpty(columns)) {
+ return String.format("%s/tables/%s/segments/%s/metadata", endpoint,
tableNameWithType, segmentName);
+ } else {
+ return String.format("%s/tables/%s/segments/%s/metadata?%s", endpoint,
tableNameWithType, segmentName,
+ generateParam(COLUMNS_KEY, columns));
+ }
}
public String generateTableMetadataServerURL(String tableNameWithType,
@Nullable List<String> columns,
- @Nullable List<String> segmentsToInclude, String endpoint) {
+ @Nullable List<String> segments, String endpoint) {
tableNameWithType = URLEncoder.encode(tableNameWithType,
StandardCharsets.UTF_8);
- String paramsStr = generateParam(COLUMNS_KEY, columns) + "&" +
generateParam(SEGMENTS_KEY, segmentsToInclude);
- return String.format("%s/tables/%s/segments/metadata?%s", endpoint,
tableNameWithType, paramsStr);
+ if (CollectionUtils.isEmpty(columns)) {
+ if (CollectionUtils.isEmpty(segments)) {
+ return String.format("%s/tables/%s/metadata", endpoint,
tableNameWithType);
+ } else {
+ return String.format("%s/tables/%s/segments/metadata?%s", endpoint,
tableNameWithType,
+ generateParam(SEGMENTS_KEY, segments));
+ }
+ } else {
+ if (CollectionUtils.isEmpty(segments)) {
+ return String.format("%s/tables/%s/metadata?%s", endpoint,
tableNameWithType,
+ generateParam(COLUMNS_KEY, columns));
+ } else {
+ return String.format("%s/tables/%s/segments/metadata?%s&%s", endpoint,
tableNameWithType,
+ generateParam(COLUMNS_KEY, columns), generateParam(SEGMENTS_KEY,
segments));
+ }
Review Comment:
Inconsistent API endpoints: when segments is empty, the method returns
`/tables/{table}/metadata` but when segments is not empty, it returns
`/tables/{table}/segments/metadata`. Based on the method name
`generateTableMetadataServerURL` and the parameter name `segments`, it should
consistently use the `/segments/metadata` endpoint when segments are provided.
```suggestion
if (CollectionUtils.isEmpty(segments)) {
if (CollectionUtils.isEmpty(columns)) {
return String.format("%s/tables/%s/metadata", endpoint,
tableNameWithType);
} else {
return String.format("%s/tables/%s/metadata?%s", endpoint,
tableNameWithType,
generateParam(COLUMNS_KEY, columns));
}
} else {
// segments is not empty, always use /segments/metadata
StringBuilder url = new
StringBuilder(String.format("%s/tables/%s/segments/metadata?", endpoint,
tableNameWithType));
boolean hasParam = false;
if (!CollectionUtils.isEmpty(columns)) {
url.append(generateParam(COLUMNS_KEY, columns));
hasParam = true;
}
if (!CollectionUtils.isEmpty(segments)) {
if (hasParam) {
url.append("&");
}
url.append(generateParam(SEGMENTS_KEY, segments));
}
return url.toString();
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
