Jackie-Jiang commented on code in PR #19378: URL: https://github.com/apache/pinot/pull/19378#discussion_r3929049212
########## pinot-plugins/pinot-input-format/pinot-protobuf/src/test/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufTempFileLeakTest.java: ########## @@ -0,0 +1,167 @@ +/** + * 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.plugin.inputformat.protobuf; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.net.URI; +import java.net.URL; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.pinot.spi.data.readers.GenericRow; +import org.apache.pinot.spi.filesystem.LocalPinotFS; +import org.apache.pinot.spi.filesystem.PinotFSFactory; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +import static org.apache.pinot.plugin.inputformat.protobuf.ProtoBufTestDataGenerator.getFieldsInSampleRecord; +import static org.apache.pinot.plugin.inputformat.protobuf.ProtoBufTestDataGenerator.getSampleRecordMessage; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; + + +/// Verifies that protobuf descriptor-based decoder and reader operations do not leak temporary directories. +/// +/// Each test snapshots the set of `pinot-protobuf*` directories in the system temp directory before the operation, +/// then asserts that no new ones remain afterward. Functional correctness of decoding is covered by existing tests +/// in [ProtoBufCodeGenMessageDecoderTest] and [ProtoBufRecordReaderTest]; these tests focus on the temp-file +/// regression only. +public class ProtoBufTempFileLeakTest { + private static final Path TEMP_DIR = Path.of(System.getProperty("java.io.tmpdir")); + private static final String REMOTE_SCHEME = "proto-test-remote"; + + @BeforeClass + public void setUp() { + PinotFSFactory.register(REMOTE_SCHEME, ClasspathPinotFS.class.getName(), null); + } + + /// [ProtoBufMessageDecoder#init] with a descriptor file should not leak a temp directory. + @Test + public void testMessageDecoderInitDoesNotLeakTempDir() + throws Exception { + Set<Path> before = listProtobufTempDirs(); + + Map<String, String> decoderProps = new HashMap<>(); + URL descriptorFile = getClass().getClassLoader().getResource("sample.desc"); Review Comment: Both consumer tests use local `file:` descriptor URIs, while the remote test closes the returned stream directly. Please configure `ProtoBufMessageDecoder` and `ProtoBufRecordReader` with `proto-test-remote:///sample.desc` and have `ClasspathPinotFS` return a close-tracking stream, so the tests prove that the consumers' try-with-resources blocks close remote streams. ########## pinot-plugins/pinot-input-format/pinot-protobuf/src/test/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufTempFileLeakTest.java: ########## @@ -0,0 +1,167 @@ +/** + * 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.plugin.inputformat.protobuf; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.net.URI; +import java.net.URL; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.pinot.spi.data.readers.GenericRow; +import org.apache.pinot.spi.filesystem.LocalPinotFS; +import org.apache.pinot.spi.filesystem.PinotFSFactory; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +import static org.apache.pinot.plugin.inputformat.protobuf.ProtoBufTestDataGenerator.getFieldsInSampleRecord; +import static org.apache.pinot.plugin.inputformat.protobuf.ProtoBufTestDataGenerator.getSampleRecordMessage; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; + + +/// Verifies that protobuf descriptor-based decoder and reader operations do not leak temporary directories. +/// +/// Each test snapshots the set of `pinot-protobuf*` directories in the system temp directory before the operation, +/// then asserts that no new ones remain afterward. Functional correctness of decoding is covered by existing tests +/// in [ProtoBufCodeGenMessageDecoderTest] and [ProtoBufRecordReaderTest]; these tests focus on the temp-file +/// regression only. +public class ProtoBufTempFileLeakTest { + private static final Path TEMP_DIR = Path.of(System.getProperty("java.io.tmpdir")); + private static final String REMOTE_SCHEME = "proto-test-remote"; + + @BeforeClass + public void setUp() { + PinotFSFactory.register(REMOTE_SCHEME, ClasspathPinotFS.class.getName(), null); + } + + /// [ProtoBufMessageDecoder#init] with a descriptor file should not leak a temp directory. + @Test + public void testMessageDecoderInitDoesNotLeakTempDir() + throws Exception { + Set<Path> before = listProtobufTempDirs(); + + Map<String, String> decoderProps = new HashMap<>(); + URL descriptorFile = getClass().getClassLoader().getResource("sample.desc"); + decoderProps.put("descriptorFile", descriptorFile.toURI().toString()); + ProtoBufMessageDecoder decoder = new ProtoBufMessageDecoder(); + decoder.init(decoderProps, getFieldsInSampleRecord(), ""); + + assertNoNewTempDirs(before); + } + + /// [ProtoBufRecordReader] lifecycle (init, read, close) should not leak a temp directory. + @Test + public void testRecordReaderLifecycleDoesNotLeakTempDir() + throws Exception { + Set<Path> before = listProtobufTempDirs(); + + File tempDataDir = Files.createTempDirectory("protobuf-leak-test-data").toFile(); + File dataFile = new File(tempDataDir, "test.data"); + try { + try (FileOutputStream out = new FileOutputStream(dataFile)) { + getSampleRecordMessage().writeDelimitedTo(out); + } + + URL descriptorFile = getClass().getClassLoader().getResource("sample.desc"); + ProtoBufRecordReaderConfig config = new ProtoBufRecordReaderConfig(); + config.setDescriptorFile(descriptorFile.toURI()); + + try (ProtoBufRecordReader reader = new ProtoBufRecordReader()) { + reader.init(dataFile, getFieldsInSampleRecord(), config); + assertTrue(reader.hasNext()); + GenericRow row = reader.next(new GenericRow()); + assertEquals(row.getValue("email"), "[email protected]"); + } + } finally { + //noinspection ResultOfMethodCallIgnored + dataFile.delete(); + //noinspection ResultOfMethodCallIgnored + tempDataDir.delete(); + } + + assertNoNewTempDirs(before); + } + + /// [ProtoBufUtils#openDescriptorFile] with a remote PinotFS scheme should return valid descriptor + /// bytes without creating any temp directories. + @Test + public void testOpenDescriptorFileWithRemoteFS() + throws Exception { + Set<Path> before = listProtobufTempDirs(); + + try (InputStream in = ProtoBufUtils.openDescriptorFile(REMOTE_SCHEME + ":///sample.desc")) { + byte[] bytes = in.readAllBytes(); + assertTrue(bytes.length > 0, "Descriptor bytes should not be empty"); + } + + assertNoNewTempDirs(before); + } + + // --------------------------------------------------------------------------- + // Helpers + // --------------------------------------------------------------------------- + + private static void assertNoNewTempDirs(Set<Path> before) Review Comment: The current Unit Test Set 2 failure demonstrates a race here: this helper scans the shared `java.io.tmpdir` and attributes any new `pinot-protobuf*` directory to this test, while parallel codegen tests create directories with the same prefix. Please have the fake PinotFS fail or count `copyToLocalFile()` calls instead of scanning global `/tmp`. -- 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]
