Jackie-Jiang commented on code in PR #16165:
URL: https://github.com/apache/pinot/pull/16165#discussion_r2164697069
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskExecutor.java:
##########
@@ -108,8 +108,9 @@ protected List<SegmentConversionResult>
convert(PinotTaskConfig pinotTaskConfig,
// Fetch validDocID snapshot from server and get record-reader for
compacted reader.
List<RecordReader> recordReaders = segmentMetadataList.stream().map(x -> {
- RoaringBitmap validDocIds =
MinionTaskUtils.getValidDocIdFromServerMatchingCrc(tableNameWithType,
x.getName(),
- ValidDocIdsType.SNAPSHOT.name(), MINION_CONTEXT, x.getCrc());
+ RoaringBitmap validDocIds =
Review Comment:
(minor) Seems the changes in this file are unnecessary
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3235,6 +3235,35 @@ public Map<String, List<String>>
getServerToSegmentsMap(String tableNameWithType
return serverToSegmentsMap;
}
+ /**
+ * Get the servers to segments map for which servers are ONLINE in external
view for those segments in IDEAL STATE
+ */
+ public Map<String, List<String>> getOnlineServerToSegmentsMap(String
tableNameWithType) {
Review Comment:
We want to make the method name more specific to reflect this one checks EV
instead of IS. We should also exclude replaced segments
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3235,6 +3235,35 @@ public Map<String, List<String>>
getServerToSegmentsMap(String tableNameWithType
return serverToSegmentsMap;
}
+ /**
+ * Get the servers to segments map for which servers are ONLINE in external
view for those segments in IDEAL STATE
+ */
+ public Map<String, List<String>> getOnlineServerToSegmentsMap(String
tableNameWithType) {
+ Map<String, List<String>> serverToSegmentsMap = new TreeMap<>();
+ IdealState idealState =
_helixAdmin.getResourceIdealState(_helixClusterName, tableNameWithType);
+ ExternalView externalView =
_helixAdmin.getResourceExternalView(_helixClusterName, tableNameWithType);
+ if (idealState == null) {
+ throw new IllegalStateException("Ideal State does not exist for table: "
+ tableNameWithType);
+ }
+ if (externalView == null) {
+ throw new IllegalStateException("External View state does not exist for
table: " + tableNameWithType);
+ }
+
+ Map<String, Map<String, String>> idealStateMap =
idealState.getRecord().getMapFields();
+
+ for (Map.Entry<String, Map<String, String>> entry :
idealStateMap.entrySet()) {
+ String segmentName = entry.getKey();
+ Map<String, String> externalViewStateMap =
externalView.getStateMap(segmentName);
Review Comment:
Need to do a `null` check here
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtils.java:
##########
@@ -236,6 +237,17 @@ public static RoaringBitmap
getValidDocIdFromServerMatchingCrc(String tableNameW
LOGGER.warn(message);
continue;
}
+
+ // skipping servers which are not in READY state. The bitmaps would be
inconsistent when
+ // server is NOT READY as UPDATING segments might be updating the ONLINE
segments
+ if
(!validDocIdsBitmapResponse.getServerStatus().equals(ServiceStatus.Status.GOOD))
{
+ String message = "Server " + validDocIdsBitmapResponse.getInstanceId()
+ " is in "
+ + validDocIdsBitmapResponse.getServerStatus() + " state, skipping
it for execution for segment: "
+ + validDocIdsBitmapResponse.getSegmentName() + ". Will try other
servers.";
+ LOGGER.error(message);
Review Comment:
Make it a warning?
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdsBitmapResponse.java:
##########
@@ -19,21 +19,27 @@
package org.apache.pinot.common.restlet.resources;
import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.pinot.common.utils.ServiceStatus;
public class ValidDocIdsBitmapResponse {
private final String _segmentName;
private final String _segmentCrc;
private final ValidDocIdsType _validDocIdsType;
private final byte[] _bitmap;
+ private final String _instanceId;
+ private final ServiceStatus.Status _serverStatus;
Review Comment:
(minor) Keep the order of declaration, constructor, getter consistent for
`ValidDocIdsBitmapResponse` and `ValidDocIdsMetadataInfo`
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtils.java:
##########
@@ -236,6 +237,17 @@ public static RoaringBitmap
getValidDocIdFromServerMatchingCrc(String tableNameW
LOGGER.warn(message);
continue;
}
+
+ // skipping servers which are not in READY state. The bitmaps would be
inconsistent when
+ // server is NOT READY as UPDATING segments might be updating the ONLINE
segments
+ if
(!validDocIdsBitmapResponse.getServerStatus().equals(ServiceStatus.Status.GOOD))
{
Review Comment:
Do we need server to be up for snapshot mode?
--
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]
