Jackie-Jiang commented on code in PR #14798: URL: https://github.com/apache/pinot/pull/14798#discussion_r1914107109
########## pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadataUtils.java: ########## @@ -0,0 +1,80 @@ +/** + * 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.metadata.segment; + +import com.fasterxml.jackson.databind.DeserializationFeature; +import com.fasterxml.jackson.databind.MapperFeature; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.SerializationFeature; +import com.fasterxml.jackson.databind.node.ObjectNode; +import java.io.IOException; +import org.apache.helix.zookeeper.datamodel.ZNRecord; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +public class SegmentZKMetadataUtils { + private SegmentZKMetadataUtils() { + } + + private static final Logger LOGGER = LoggerFactory.getLogger(SegmentZKMetadataUtils.class); Review Comment: (nit) Not used ########## pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java: ########## @@ -1248,6 +1249,63 @@ public File downloadUntarFileStreamed(URI uri, File dest, AuthProvider authProvi httpHeaders, maxStreamRateInByte); } + /** + * Invokes the server's reIngestSegment API via a POST request with JSON payload, + * using Simple HTTP APIs. + * + * POST http://[serverURL]/reIngestSegment + * { + * "tableNameWithType": [tableName], + * "segmentName": [segmentName], + * "uploadURI": [leadControllerUrl], + * "uploadSegment": true + * } + */ + //TODO: Add auth and https support + public void triggerReIngestion(String serverHostPort, String tableNameWithType, String segmentName, + String leadControllerUrl) + throws IOException, URISyntaxException, HttpErrorStatusException { + + + if (serverHostPort.contains("http://";)) { + serverHostPort = serverHostPort.replace("http://";, ""); + } + + String serverHost = serverHostPort.split(":")[0]; + String serverPort = serverHostPort.split(":")[1]; + + URI reIngestUri = getURI(HTTP, serverHost, Integer.parseInt(serverPort), REINGEST_SEGMENT_PATH); + + // Build the JSON payload + Map<String, Object> requestJson = new HashMap<>(); + requestJson.put("tableNameWithType", tableNameWithType); + requestJson.put("segmentName", segmentName); + requestJson.put("uploadURI", leadControllerUrl); Review Comment: What if the lead controller is gone? Can we use the lead controller locator on the server side? ########## pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java: ########## @@ -1248,6 +1249,63 @@ public File downloadUntarFileStreamed(URI uri, File dest, AuthProvider authProvi httpHeaders, maxStreamRateInByte); } + /** + * Invokes the server's reIngestSegment API via a POST request with JSON payload, + * using Simple HTTP APIs. + * + * POST http://[serverURL]/reIngestSegment + * { + * "tableNameWithType": [tableName], + * "segmentName": [segmentName], + * "uploadURI": [leadControllerUrl], + * "uploadSegment": true + * } + */ + //TODO: Add auth and https support + public void triggerReIngestion(String serverHostPort, String tableNameWithType, String segmentName, + String leadControllerUrl) + throws IOException, URISyntaxException, HttpErrorStatusException { + + + if (serverHostPort.contains("http://";)) { + serverHostPort = serverHostPort.replace("http://";, ""); + } + + String serverHost = serverHostPort.split(":")[0]; + String serverPort = serverHostPort.split(":")[1]; + + URI reIngestUri = getURI(HTTP, serverHost, Integer.parseInt(serverPort), REINGEST_SEGMENT_PATH); + + // Build the JSON payload + Map<String, Object> requestJson = new HashMap<>(); + requestJson.put("tableNameWithType", tableNameWithType); + requestJson.put("segmentName", segmentName); + requestJson.put("uploadURI", leadControllerUrl); + requestJson.put("uploadSegment", true); Review Comment: Will this ever to false? ########## pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java: ########## @@ -125,6 +126,7 @@ public static FileUploadType getDefaultUploadType() { private static final String FORCE_CLEANUP_PARAMETER = "&forceCleanup="; private static final String RETENTION_PARAMETER = "retention="; + public static final String REINGEST_SEGMENT_PATH = "/reIngestSegment"; Review Comment: (minor) ```suggestion public static final String REINGEST_SEGMENT_PATH = "/reingestSegment"; ``` ########## pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java: ########## @@ -125,6 +126,7 @@ public static FileUploadType getDefaultUploadType() { private static final String FORCE_CLEANUP_PARAMETER = "&forceCleanup="; private static final String RETENTION_PARAMETER = "retention="; + public static final String REINGEST_SEGMENT_PATH = "/reIngestSegment"; Review Comment: This is personal preference, but I feel `reingest` is one work ########## pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadataUtils.java: ########## @@ -0,0 +1,80 @@ +/** + * 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.metadata.segment; + +import com.fasterxml.jackson.databind.DeserializationFeature; +import com.fasterxml.jackson.databind.MapperFeature; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.SerializationFeature; +import com.fasterxml.jackson.databind.node.ObjectNode; +import java.io.IOException; +import org.apache.helix.zookeeper.datamodel.ZNRecord; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +public class SegmentZKMetadataUtils { + private SegmentZKMetadataUtils() { + } + + private static final Logger LOGGER = LoggerFactory.getLogger(SegmentZKMetadataUtils.class); + public static final ObjectMapper MAPPER = createObjectMapper(); + + private static ObjectMapper createObjectMapper() { + ObjectMapper mapper = new ObjectMapper(); + mapper.configure(SerializationFeature.INDENT_OUTPUT, true); + mapper.configure(MapperFeature.AUTO_DETECT_FIELDS, true); + mapper.configure(MapperFeature.AUTO_DETECT_SETTERS, true); + mapper.configure(MapperFeature.CAN_OVERRIDE_ACCESS_MODIFIERS, true); + mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); + return mapper; + } + + public static String serialize(SegmentZKMetadata metadata) throws IOException { Review Comment: (format) Please apply [Pinot Style](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#intellij) and reformat all the changes ########## pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java: ########## @@ -968,26 +970,25 @@ public String uploadToSegmentStore(String uri) * Used by controllers to send requests to servers: Controller periodic task uses this endpoint to ask servers * to upload committed llc segment to segment store if missing. * @param uri The uri to ask servers to upload segment to segment store - * @return {@link TableLLCSegmentUploadResponse} - segment download url, crc, other metadata + * @return {@link SegmentZKMetadata} - segment download url, crc, other metadata * @throws URISyntaxException * @throws IOException * @throws HttpErrorStatusException */ - public TableLLCSegmentUploadResponse uploadLLCToSegmentStore(String uri) + public SegmentZKMetadata uploadLLCToSegmentStore(String uri) Review Comment: This change is backward incompatible. You might need to add a new API for this ########## pinot-common/src/main/java/org/apache/pinot/common/metadata/segment/SegmentZKMetadataUtils.java: ########## @@ -0,0 +1,80 @@ +/** + * 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.metadata.segment; + +import com.fasterxml.jackson.databind.DeserializationFeature; +import com.fasterxml.jackson.databind.MapperFeature; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.SerializationFeature; +import com.fasterxml.jackson.databind.node.ObjectNode; +import java.io.IOException; +import org.apache.helix.zookeeper.datamodel.ZNRecord; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +public class SegmentZKMetadataUtils { + private SegmentZKMetadataUtils() { + } + + private static final Logger LOGGER = LoggerFactory.getLogger(SegmentZKMetadataUtils.class); + public static final ObjectMapper MAPPER = createObjectMapper(); + + private static ObjectMapper createObjectMapper() { Review Comment: Any specific reason for adding this custom mapper instead of using the `JsonUtils`? This change is independent of failure handling, so if this is really required, can you put this as a separate PR? ########## pinot-common/src/main/java/org/apache/pinot/common/utils/FileUploadDownloadClient.java: ########## @@ -1248,6 +1249,63 @@ public File downloadUntarFileStreamed(URI uri, File dest, AuthProvider authProvi httpHeaders, maxStreamRateInByte); } + /** + * Invokes the server's reIngestSegment API via a POST request with JSON payload, + * using Simple HTTP APIs. + * + * POST http://[serverURL]/reIngestSegment + * { + * "tableNameWithType": [tableName], + * "segmentName": [segmentName], + * "uploadURI": [leadControllerUrl], + * "uploadSegment": true + * } + */ + //TODO: Add auth and https support + public void triggerReIngestion(String serverHostPort, String tableNameWithType, String segmentName, + String leadControllerUrl) + throws IOException, URISyntaxException, HttpErrorStatusException { + + + if (serverHostPort.contains("http://";)) { + serverHostPort = serverHostPort.replace("http://";, ""); + } + + String serverHost = serverHostPort.split(":")[0]; + String serverPort = serverHostPort.split(":")[1]; + + URI reIngestUri = getURI(HTTP, serverHost, Integer.parseInt(serverPort), REINGEST_SEGMENT_PATH); + + // Build the JSON payload + Map<String, Object> requestJson = new HashMap<>(); + requestJson.put("tableNameWithType", tableNameWithType); + requestJson.put("segmentName", segmentName); + requestJson.put("uploadURI", leadControllerUrl); + requestJson.put("uploadSegment", true); + + // Convert the request payload to JSON string + String jsonPayload = JsonUtils.objectToString(requestJson); + // Prepare a POST request with Simple HTTP + ClassicRequestBuilder requestBuilder = ClassicRequestBuilder Review Comment: Do we need to construct it like this? There should be utils to help send the request -- 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]
