Jackie-Jiang commented on code in PR #14612:
URL: https://github.com/apache/pinot/pull/14612#discussion_r1878946618
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/TableSegmentValidationInfo.java:
##########
@@ -41,4 +44,8 @@ public boolean isValid() {
public long getMaxEndTimeMs() {
return _maxEndTimeMs;
}
+
+ public String getInvalidReason() {
Review Comment:
(minor) Annotate it as `@Nullable`
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/TableSegmentValidationInfo.java:
##########
@@ -25,12 +25,15 @@
public class TableSegmentValidationInfo {
private final boolean _valid;
+ private final String _invalidReason;
private final long _maxEndTimeMs;
@JsonCreator
public TableSegmentValidationInfo(@JsonProperty("valid") boolean valid,
+ @JsonProperty("invalidReason") String invalidReason,
Review Comment:
(minor) Annotate it as `@Nullable`
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java:
##########
@@ -1033,7 +1033,10 @@ public TableSegmentValidationInfo
validateTableSegmentState(
case SegmentStateModel.CONSUMING:
// Only validate presence of segment
if (segmentDataManager == null) {
- return new TableSegmentValidationInfo(false, -1);
+ String invalidReason = String.format(
+ "Segment %s is in CONSUMING state, but segmentDataManager
is null", segmentName);
+ LOGGER.error(invalidReason);
Review Comment:
Let's not log error here. This is normal case, and returning the invalid
reason through response should be good enough. Same for other places
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -1256,8 +1256,9 @@ private long validateSegmentStateForTable(String
offlineTableName)
TableSegmentValidationInfo tableSegmentValidationInfo =
JsonUtils.stringToObject(response, TableSegmentValidationInfo.class);
if (!tableSegmentValidationInfo.isValid()) {
- throw new ControllerApplicationException(LOGGER, "Table segment
validation failed",
- Response.Status.PRECONDITION_FAILED);
+ String error = String.format("Table segment validation failed.
error=%s",
Review Comment:
(minor) We have started an effort to avoid using `String.format()` and
replace it with regular concatenation. Same for other places
--
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]
