Copilot commented on code in PR #15015:
URL: https://github.com/apache/pinot/pull/15015#discussion_r2106272540
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotIngestionRestletResourceStatelessTest.java:
##########
@@ -77,12 +85,20 @@ public void setUp()
// Create a file with few records
_inputFile = new File(FileUtils.getTempDirectory(),
"pinotIngestionRestletResourceTest_data.csv");
+ _fileContent = String.join("\n",
+ "breed|name",
+ "dog|cooper",
+ "cat|kylo",
+ "dog|cookie"
+ );
try (BufferedWriter bw = new BufferedWriter(new FileWriter(_inputFile))) {
- bw.write("breed|name\n");
- bw.write("dog|cooper\n");
- bw.write("cat|kylo\n");
- bw.write("dog|cookie\n");
+ bw.write(_fileContent);
}
+
+ _dummyServer = HttpServer.create();
+ _dummyServer.bind(new InetSocketAddress("localhost", 0), 0);
+ _dummyServer.start();
+ _dummyServer.createContext("/mock/ingestion", new
SegmentAsCsvFileFromPublicUriHandler());
Review Comment:
Contexts should be created before `HttpServer.start()`; move `createContext`
above the `start()` call to ensure the handler is registered before the server
begins accepting requests.
```suggestion
_dummyServer.createContext("/mock/ingestion", new
SegmentAsCsvFileFromPublicUriHandler());
_dummyServer.start();
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/FileIngestionHelper.java:
##########
@@ -111,12 +114,22 @@ public SuccessResponse buildSegmentAndPush(DataPayload
payload)
// Copy file to local working dir
File inputFile = new File(inputDir, String.format(
"%s.%s", DATA_FILE_PREFIX,
_batchConfigMap.get(BatchConfigProperties.INPUT_FORMAT).toLowerCase()));
- if (payload._payloadType == PayloadType.URI) {
- copyURIToLocal(_batchConfigMap, payload._uri, inputFile);
- LOGGER.info("Copied from URI: {} to local file: {}", payload._uri,
inputFile.getAbsolutePath());
- } else {
- copyMultipartToLocal(payload._multiPart, inputFile);
- LOGGER.info("Copied multipart payload to local file: {}",
inputDir.getAbsolutePath());
+ switch (payload._payloadType) {
+ case BUCKET_URI: {
+ copyBucketURIToLocal(_batchConfigMap, payload._uri, inputFile);
+ LOGGER.info("Copied from bucket URI: {} to local file: {}",
payload._uri, inputFile.getAbsolutePath());
+ break;
+ }
+ case PUBLIC_URI: {
+ copyPublicURIToLocal(payload._uri, inputFile);
+ LOGGER.info("Copied from public URI: {} to local file: {}",
payload._uri, inputDir.getAbsolutePath());
Review Comment:
The log prints `inputDir.getAbsolutePath()` instead of the destination file
path. It should reference `destFile.getAbsolutePath()` to reflect the correct
target file.
```suggestion
LOGGER.info("Copied from public URI: {} to local file: {}",
payload._uri, inputFile.getAbsolutePath());
```
##########
pinot-controller/src/test/java/org/apache/pinot/controller/api/PinotIngestionRestletResourceStatelessTest.java:
##########
@@ -133,5 +156,22 @@ public void tearDown() {
stopFakeInstances();
stopController();
stopZk();
+ if (_dummyServer != null) {
+ _dummyServer.stop(0);
+ }
+ }
+
+ private class SegmentAsCsvFileFromPublicUriHandler implements HttpHandler {
+ @Override
+ public void handle(HttpExchange exchange)
+ throws IOException {
+ exchange.sendResponseHeaders(200, 0);
+ OutputStream out = exchange.getResponseBody();
+ OutputStreamWriter writer = new OutputStreamWriter(out);
+ writer.append(_fileContent);
+ writer.flush();
+ out.flush();
+ out.close();
Review Comment:
The `OutputStreamWriter` is not closed explicitly. Wrap it in a
try-with-resources or call `writer.close()` to avoid resource leaks.
```suggestion
try (OutputStream out = exchange.getResponseBody();
OutputStreamWriter writer = new OutputStreamWriter(out)) {
writer.append(_fileContent);
writer.flush();
out.flush();
}
```
--
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]
