Jackie-Jiang commented on code in PR #13789: URL: https://github.com/apache/pinot/pull/13789#discussion_r1731723321
########## pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/ServerSegmentsReloadCheckResponse.java: ########## @@ -0,0 +1,53 @@ +/** + * 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.JsonIgnoreProperties; +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. + */ +@JsonIgnoreProperties(ignoreUnknown = true) +public class ServerSegmentsReloadCheckResponse { + @JsonProperty("needReload") + private final boolean _needReload; + + @JsonProperty("instanceId") + private final String _instanceId; + + public String getInstanceId() { + return _instanceId; + } + + public boolean getNeedReload() { Review Comment: ```suggestion public boolean isNeedReload() { ``` ########## pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/ServerSegmentsReloadCheckResponse.java: ########## @@ -0,0 +1,53 @@ +/** + * 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.JsonIgnoreProperties; +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. + */ +@JsonIgnoreProperties(ignoreUnknown = true) +public class ServerSegmentsReloadCheckResponse { + @JsonProperty("needReload") + private final boolean _needReload; + + @JsonProperty("instanceId") + private final String _instanceId; + + public String getInstanceId() { + return _instanceId; + } Review Comment: (nit) Suggest keeping the getters order the same as variable definition ########## pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/TableSegmentsReloadCheckResponse.java: ########## @@ -0,0 +1,55 @@ +/** + * 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.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonProperty; +import java.util.Map; + + +/** + * This class gives list of the details from each server if there exists any segments that need to be reloaded + * + * It has details of reload flag which returns true if reload is needed on table and additional details of the + * respective servers. + */ +@JsonIgnoreProperties(ignoreUnknown = true) +public class TableSegmentsReloadCheckResponse { + @JsonProperty("needReload") + boolean _needReload; + @JsonProperty("serverToSegmentsCheckReloadList") + Map<String, ServerSegmentsReloadCheckResponse> _serverToSegmentsCheckReloadList; + + public Map<String, ServerSegmentsReloadCheckResponse> getServerToSegmentsCheckReloadList() { Review Comment: (nit) Suggest keeping the getters order the same as variable definition ########## pinot-common/src/test/java/org/apache/pinot/common/utils/SerializerResponseTest.java: ########## @@ -0,0 +1,90 @@ +/** + * 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.utils; + +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.JsonNode; +import java.io.IOException; +import java.util.HashMap; +import java.util.Map; +import org.apache.pinot.common.restlet.resources.ServerSegmentsReloadCheckResponse; +import org.apache.pinot.common.restlet.resources.TableSegmentsReloadCheckResponse; +import org.apache.pinot.spi.utils.JsonUtils; +import org.testng.annotations.Test; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertTrue; + + +/** + * Tests some of the serializer and deserialization responses from SegmentsReloadCheckResponse class + * needReload will have to be carefully evaluated + */ +public class SerializerResponseTest { Review Comment: Rename it to `SegmentsReloadCheckResponseTest` ########## pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java: ########## @@ -239,6 +239,11 @@ public String forTableReload(String tableName, TableType tableType, boolean forc return StringUtil.join("/", _baseUrl, "segments", tableName, query); } + public String forTableNeedReload(String tableNameWithType) { + String query = String.format("needReload?verbose=%s", "false"); Review Comment: (minor) No need to format ########## pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java: ########## @@ -721,6 +721,11 @@ public String reloadOfflineTable(String tableName, boolean forceDownload) return getControllerRequestClient().reloadTable(tableName, TableType.OFFLINE, forceDownload); } + public String needReloadOfflineTable(String tableNameWithType) Review Comment: Rename it to `checkIfReloadIsNeeded`. Does it only apply to offline table? ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java: ########## @@ -173,6 +175,16 @@ private File getValidDocIdsSnapshotFile() { V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME); } + public boolean needReloadSegment(String segmentName, String crc, IndexLoadingConfig indexLoadingConfig) Review Comment: Suggest renaming it to `isReloadNeeded()` ########## pinot-controller/src/main/java/org/apache/pinot/controller/util/TableMetadataReader.java: ########## @@ -55,6 +61,27 @@ public TableMetadataReader(Executor executor, HttpClientConnectionManager connec _pinotHelixResourceManager = helixResourceManager; } + public Map<String, JsonNode> getServerCheckSegmentsReloadMetadata(String tableName, TableType tableType, + int timeoutMs) + throws InvalidConfigException, IOException { + String tableNameWithType = Review Comment: This check should be performed in the `RestletResource` class before calling this method ########## pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java: ########## @@ -822,6 +824,48 @@ public String getServerMetadata( return segmentsMetadata; } + @GET + @Path("segments/{tableNameWithType}/needReload") + @Authorize(targetType = TargetType.TABLE, paramName = "tableNameWithType", action = Actions.Table.GET_METADATA) + @Produces(MediaType.APPLICATION_JSON) + @ApiOperation(value = "Gets the metadata of reload segments check from servers hosting the table", notes = + "Returns true if " + "reload is needed on the table from any one of the servers") + public String getTableReloadMetadata( + @ApiParam(value = "Table name with type", required = true, example = "myTable_REALTIME") + @PathParam("tableNameWithType") String tableNameWithType, + @QueryParam("verbose") @DefaultValue("false") boolean verbose, @Context HttpHeaders headers) { + String tableName = TableNameBuilder.extractRawTableName(tableNameWithType); Review Comment: You'll need to translate the table name based on the database name. See other method for examples ########## pinot-controller/src/main/java/org/apache/pinot/controller/helix/ControllerRequestClient.java: ########## @@ -201,6 +201,18 @@ public String reloadTable(String tableName, TableType tableType, boolean forceDo } } + public String needReloadTable(String tableNameWithType) Review Comment: Suggest renaming it to `checkIfReloadIsNeeded` ########## pinot-controller/src/main/java/org/apache/pinot/controller/util/TableMetadataReader.java: ########## @@ -55,6 +61,27 @@ public TableMetadataReader(Executor executor, HttpClientConnectionManager connec _pinotHelixResourceManager = helixResourceManager; } + public Map<String, JsonNode> getServerCheckSegmentsReloadMetadata(String tableName, TableType tableType, Review Comment: Directly pass in `tableNameWithType` to b consistent with other methods ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java: ########## @@ -173,6 +175,16 @@ private File getValidDocIdsSnapshotFile() { V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME); } + public boolean needReloadSegment(String segmentName, String crc, IndexLoadingConfig indexLoadingConfig) + throws Exception { + Schema schema = indexLoadingConfig.getSchema(); + //if re processing or reload is needed on a segment then return true + if (_segmentDirectory != null) { + return ImmutableSegmentLoader.needPreprocess(_segmentDirectory, indexLoadingConfig, schema); + } + return false; Review Comment: (minor) The null check is redundant ```suggestion return ImmutableSegmentLoader.needPreprocess(_segmentDirectory, indexLoadingConfig, indexLoadingConfig.getSchema()); ``` ########## pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java: ########## @@ -822,6 +824,48 @@ public String getServerMetadata( return segmentsMetadata; } + @GET + @Path("segments/{tableNameWithType}/needReload") + @Authorize(targetType = TargetType.TABLE, paramName = "tableNameWithType", action = Actions.Table.GET_METADATA) + @Produces(MediaType.APPLICATION_JSON) + @ApiOperation(value = "Gets the metadata of reload segments check from servers hosting the table", notes = + "Returns true if " + "reload is needed on the table from any one of the servers") Review Comment: ```suggestion "Returns true if reload is needed on the table from any one of the servers") ``` ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java: ########## @@ -173,6 +175,16 @@ private File getValidDocIdsSnapshotFile() { V1Constants.VALID_DOC_IDS_SNAPSHOT_FILE_NAME); } + public boolean needReloadSegment(String segmentName, String crc, IndexLoadingConfig indexLoadingConfig) Review Comment: Seems `crc` is not needed? -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
