swaminathanmanish commented on code in PR #14451: URL: https://github.com/apache/pinot/pull/14451#discussion_r1855428433
########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/data/manager/NeedRefreshResponse.java: ########## @@ -0,0 +1,62 @@ +/** + * 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.segment.local.data.manager; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.annotation.JsonProperty; + + +/** + * Encapsulates the response to get list of segments that need to be refreshed. The response also contains the reason + * why a segment has to be refreshed. + */ +public class NeedRefreshResponse { + private final String _segmentName; + private final boolean _needRefresh; + private final String _reason; Review Comment: Good idea to add reason as well ! ########## pinot-controller/src/main/java/org/apache/pinot/controller/util/ServerSegmentMetadataReader.java: ########## @@ -397,6 +398,39 @@ public ValidDocIdsBitmapResponse getValidDocIdsBitmapFromServer(String tableName return response; } + public Map<String, List<NeedRefreshResponse>> getSegmentsForRefreshFromServer( + String tableNameWithType, Set<String> serverInstances, BiMap<String, String> endpoints, int timeoutMs) { + LOGGER.debug("Getting list of segments for refresh from servers for table {}.", tableNameWithType); + List<String> serverURLs = new ArrayList<>(); + for (String serverInstance : serverInstances) { + serverURLs.add(generateNeedRefreshSegmentsServerURL(tableNameWithType, endpoints.get(serverInstance))); + } + BiMap<String, String> endpointsToServers = endpoints.inverse(); + CompletionServiceHelper completionServiceHelper = + new CompletionServiceHelper(_executor, _connectionManager, endpointsToServers); + CompletionServiceHelper.CompletionServiceResponse serviceResponse = + completionServiceHelper.doMultiGetRequest(serverURLs, tableNameWithType, true, timeoutMs); + Map<String, List<NeedRefreshResponse>> serverResponses = new HashMap<>(); + + int failedParses = 0; + for (Map.Entry<String, String> streamResponse : serviceResponse._httpResponses.entrySet()) { + try { + // TODO: RV - get the instance name instead of the endpoint + serverResponses.put(streamResponse.getKey(), + JsonUtils.stringToObject(streamResponse.getValue(), + new TypeReference<List<NeedRefreshResponse>>() { })); + } catch (Exception e) { + failedParses++; + LOGGER.error("Unable to parse server {} response due to an error: ", streamResponse.getKey(), e); + } + } + if (failedParses != 0) { + LOGGER.error("Unable to parse server {} / {} response due to an error: ", failedParses, serverURLs.size()); Review Comment: Please add table name to this msg ########## pinot-controller/src/main/java/org/apache/pinot/controller/util/ServerSegmentMetadataReader.java: ########## @@ -397,6 +398,39 @@ public ValidDocIdsBitmapResponse getValidDocIdsBitmapFromServer(String tableName return response; } + public Map<String, List<NeedRefreshResponse>> getSegmentsForRefreshFromServer( + String tableNameWithType, Set<String> serverInstances, BiMap<String, String> endpoints, int timeoutMs) { + LOGGER.debug("Getting list of segments for refresh from servers for table {}.", tableNameWithType); + List<String> serverURLs = new ArrayList<>(); + for (String serverInstance : serverInstances) { + serverURLs.add(generateNeedRefreshSegmentsServerURL(tableNameWithType, endpoints.get(serverInstance))); + } + BiMap<String, String> endpointsToServers = endpoints.inverse(); + CompletionServiceHelper completionServiceHelper = + new CompletionServiceHelper(_executor, _connectionManager, endpointsToServers); + CompletionServiceHelper.CompletionServiceResponse serviceResponse = + completionServiceHelper.doMultiGetRequest(serverURLs, tableNameWithType, true, timeoutMs); + Map<String, List<NeedRefreshResponse>> serverResponses = new HashMap<>(); + + int failedParses = 0; + for (Map.Entry<String, String> streamResponse : serviceResponse._httpResponses.entrySet()) { + try { + // TODO: RV - get the instance name instead of the endpoint + serverResponses.put(streamResponse.getKey(), + JsonUtils.stringToObject(streamResponse.getValue(), + new TypeReference<List<NeedRefreshResponse>>() { })); + } catch (Exception e) { + failedParses++; + LOGGER.error("Unable to parse server {} response due to an error: ", streamResponse.getKey(), e); Review Comment: Please add table name to this msg ########## pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java: ########## @@ -891,6 +892,31 @@ public String getTableReloadMetadata( } } + @GET + @Path("segments/{tableNameWithType}/needRefresh") + @Authorize(targetType = TargetType.TABLE, paramName = "tableNameWithType", action = Actions.Table.GET_METADATA) + @Produces(MediaType.APPLICATION_JSON) + @ApiOperation(value = "Gets a list of segments that need to be refreshed from servers hosting the table", notes = + "Gets a list of segments that need to be refreshed from servers hosting the table") + public Map<String, List<NeedRefreshResponse>> getTableRefreshMetadata( Review Comment: How will the user know if we got partial Vs full response. Would it be useful to find out missing segments for which we never got a response? After the getSegmentsForRefreshFromServer, we can get the list of segments from ideal state and do a diff. If there's a way to propagate that list, we can just retry that list in the subsequent call. Or at least the client knows that it did not get a complete list. ########## pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java: ########## @@ -1046,6 +1064,261 @@ public boolean needReloadSegments() return needReload; } + @Override + public List<NeedRefreshResponse> getSegmentsForRefresh(TableConfig tableConfig, Schema schema) { + List<NeedRefreshResponse> segmentsRequiringRefresh = new ArrayList<>(); + List<SegmentDataManager> segmentDataManagers = acquireAllSegments(); Review Comment: Good point about acquireAll. Yes thats one way. If we have a large table/with a large number of segments, this check can take time. However by the time you get to the segment, that can be released? I guess acquireSegment will return null in that case. -- 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
