Jackie-Jiang commented on code in PR #13789:
URL: https://github.com/apache/pinot/pull/13789#discussion_r1719157723
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -1013,6 +1013,22 @@ public boolean tryLoadExistingSegment(SegmentZKMetadata
zkMetadata, IndexLoading
}
}
+ boolean needReloadSegment(SegmentZKMetadata zkMetadata, IndexLoadingConfig
indexLoadingConfig)
+ throws Exception {
+ 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) {
Review Comment:
We can probably directly throw the exception out without catch
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -1024,6 +1040,28 @@ private SegmentDirectory tryInitSegmentDirectory(String
segmentName, String segm
}
}
+ @Override
+ public boolean needReloadSegments()
+ throws Exception {
+ IndexLoadingConfig indexLoadingConfig = fetchIndexLoadingConfig();
+ List<SegmentDataManager> segmentDataManagers = acquireAllSegments();
+ boolean needReload = false;
+ try {
+ for (SegmentDataManager segmentDataManager : segmentDataManagers) {
+ SegmentZKMetadata segmentZKMetadata =
fetchZKMetadata(segmentDataManager.getSegmentName());
Review Comment:
Let's not read ZK metadata because it needs to access ZK, and we might end
up reading it for thousands of segments.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -1013,6 +1013,22 @@ public boolean tryLoadExistingSegment(SegmentZKMetadata
zkMetadata, IndexLoading
}
}
+ boolean needReloadSegment(SegmentZKMetadata zkMetadata, IndexLoadingConfig
indexLoadingConfig)
+ throws Exception {
+ String segmentName = zkMetadata.getSegmentName();
+ SegmentDirectory segmentDirectory =
+ tryInitSegmentDirectory(segmentName,
String.valueOf(zkMetadata.getCrc()), indexLoadingConfig);
Review Comment:
This can cause exception for consuming segments. We should only check
immutable segments. We can read the `SegmentDirectory` out from
`ImmutableSegmentImpl`. We can consider adding a test in
`BaseClusterIntegrationTestSet.testReload()` to verify if the API works. You
may also add the test after the controller side integration.
##########
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 _needReload;
+ private final String _serverInstanceId;
Review Comment:
(minor) `instanceId` might be more concise because this is always returned
from server.
Do we really need to return it? Controller should already have this info
when reading the address of the server
##########
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 _needReload;
+ private final String _serverInstanceId;
+
+ public String getServerInstanceId() {
+ return _serverInstanceId;
+ }
+
+ public boolean getReload() {
Review Comment:
This method name might not work for JSON serialization. I think it will be
mistakenly serialized as `reload`. Can you add a test to verify? We might need
to rename it to `isNeedReload()`, or explicitly annotate it with
`@JsonProperty("needReload")`. If we explicitly annotate, a better name could
be `isReloadNeeded()`
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -1013,6 +1013,22 @@ public boolean tryLoadExistingSegment(SegmentZKMetadata
zkMetadata, IndexLoading
}
}
+ boolean needReloadSegment(SegmentZKMetadata zkMetadata, IndexLoadingConfig
indexLoadingConfig)
Review Comment:
Make it `private`
--
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
