jackjlli commented on code in PR #13789: URL: https://github.com/apache/pinot/pull/13789#discussion_r1716111730
########## pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/SegmentsReloadCheckResponse.java: ########## @@ -0,0 +1,48 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.common.restlet.resources; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; + + +/** + * This class gives the data of a server if there exists any segments that need to be reloaded + * + * It has details of server id and returns true/false if there are any segments to be reloaded or not. + */ +public class SegmentsReloadCheckResponse { + private final boolean _isReload; + private final String _serverInstanceId; + + @JsonCreator + public SegmentsReloadCheckResponse(@JsonProperty("isReload") boolean isReload, + @JsonProperty("serverInstanceId") String serverInstanceId) { Review Comment: I wonder why the `serverInstanceId` needs to be returned as part of the response payload here? Didn't we already know where the request was sent to? ########## pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java: ########## @@ -952,4 +953,23 @@ public TableSegmentValidationInfo validateTableSegmentState( } return new TableSegmentValidationInfo(true, maxEndTimeMs); } + + @GET + @Path("/tables/{tableName}/segments/reload") + @Produces(MediaType.APPLICATION_JSON) + @ApiOperation(value = "Checks if reload is needed on any segment", notes = "Returns true if reload is required on" + + " any segment in this server") + @ApiResponses(value = { + @ApiResponse(code = 200, message = "Success", response = TableSegments.class), @ApiResponse(code = 500, + message = "Server initialization error", response = ErrorInfo.class) + }) + public String checkSegmentsReload( + @ApiParam(value = "Table Name with type", required = true) @PathParam("tableName") String tableName, + @Context HttpHeaders headers) { + tableName = DatabaseUtils.translateTableName(tableName, headers); + TableDataManager tableDataManager = ServerResourceUtils.checkGetTableDataManager(_serverInstance, tableName); + boolean isSegmentsReload = tableDataManager.needReloadSegments(); Review Comment: nit: rename the output to sth like `needReload`? ########## pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/SegmentsReloadCheckResponse.java: ########## @@ -0,0 +1,48 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.common.restlet.resources; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; + + +/** + * This class gives the data of a server if there exists any segments that need to be reloaded + * + * It has details of server id and returns true/false if there are any segments to be reloaded or not. + */ +public class SegmentsReloadCheckResponse { + private final boolean _isReload; + private final String _serverInstanceId; + + @JsonCreator + public SegmentsReloadCheckResponse(@JsonProperty("isReload") boolean isReload, Review Comment: nit: `needReload` instead? ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/data/manager/TableDataManager.java: ########## @@ -122,6 +122,19 @@ void addNewOnlineSegment(SegmentZKMetadata zkMetadata, IndexLoadingConfig indexL */ boolean tryLoadExistingSegment(SegmentZKMetadata zkMetadata, IndexLoadingConfig indexLoadingConfig); + /** + * + * Checks for a particular segment, reload is needed or not + * @return true if the reload is needed on the segment + */ + boolean checkReloadSegment(SegmentZKMetadata zkMetadata, IndexLoadingConfig indexLoadingConfig); Review Comment: nit: unify the method name to sth like `needReloadSegment(...)`? ########## pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java: ########## @@ -1024,6 +1046,27 @@ private SegmentDirectory tryInitSegmentDirectory(String segmentName, String segm } } + @Override + public boolean needReloadSegments() { + IndexLoadingConfig indexLoadingConfig = fetchIndexLoadingConfig(); + List<SegmentDataManager> segmentDataManagers = acquireAllSegments(); + boolean segmentsReloadCheck = false; Review Comment: nit: rename it to sth like `needSegmentReload` ########## pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java: ########## @@ -1013,8 +1013,30 @@ public boolean tryLoadExistingSegment(SegmentZKMetadata zkMetadata, IndexLoading } } + /** + * This method checks if any reload is needed on a segment. + * Scans through indices, columns, startree index and min max values to achieve the result + * True if reload is needed and false if no reload is needed. + */ + @Override + public boolean checkReloadSegment(SegmentZKMetadata zkMetadata, IndexLoadingConfig indexLoadingConfig) { + String segmentName = zkMetadata.getSegmentName(); + SegmentDirectory segmentDirectory = + tryInitSegmentDirectory(segmentName, String.valueOf(zkMetadata.getCrc()), indexLoadingConfig); + + try { + Schema schema = indexLoadingConfig.getSchema(); + //if re processing or reload is needed on a segment then return true + return ImmutableSegmentLoader.needPreprocess(segmentDirectory, indexLoadingConfig, schema); + } catch (Exception e) { + _logger.error("Failed to check if reload is needed for a segment {}", segmentName, e); + closeSegmentDirectoryQuietly(segmentDirectory); + return false; Review Comment: I saw you stated in the API description that `500` will be returned if there is any server side error. Shouldn't we return `500` here in this case? ########## pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java: ########## @@ -1013,8 +1013,30 @@ public boolean tryLoadExistingSegment(SegmentZKMetadata zkMetadata, IndexLoading } } + /** + * This method checks if any reload is needed on a segment. + * Scans through indices, columns, startree index and min max values to achieve the result + * True if reload is needed and false if no reload is needed. + */ + @Override + public boolean checkReloadSegment(SegmentZKMetadata zkMetadata, IndexLoadingConfig indexLoadingConfig) { + String segmentName = zkMetadata.getSegmentName(); + SegmentDirectory segmentDirectory = + tryInitSegmentDirectory(segmentName, String.valueOf(zkMetadata.getCrc()), indexLoadingConfig); + + try { + Schema schema = indexLoadingConfig.getSchema(); + //if re processing or reload is needed on a segment then return true + return ImmutableSegmentLoader.needPreprocess(segmentDirectory, indexLoadingConfig, schema); + } catch (Exception e) { + _logger.error("Failed to check if reload is needed for a segment {}", segmentName, e); Review Comment: Can we emit a server metric to indicate that something is wrong when running the check? -- 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]
