mayankshriv commented on code in PR #19378: URL: https://github.com/apache/pinot/pull/19378#discussion_r3929429013
########## 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: You are right, and I was wrong to call that CI failure unrelated - the codegen tests in this same module create `pinot-protobuf*` directories via `getFileCopiedToLocal`, so the global `/tmp` scan attributes them to whichever test happens to snapshot around them. Removed the `/tmp` scanning entirely. `ClasspathPinotFS` now counts `open()` and `copyToLocalFile()` calls, and the assertion is `copyToLocalFileCount == 0` - which is a direct statement of the regression (the old `getDescriptorFileInputStream` staged the descriptor through `copyToLocalFile`) rather than an inference from shared filesystem state. Counters reset per method via `@BeforeMethod`, so nothing depends on other tests. ########## 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: Done. Both consumers are now configured with `proto-test-remote:///sample.desc`, so they actually exercise the non-`file:` path through `openDescriptorFile` rather than shortcutting to a local file, and the standalone stream test is gone (it was proving the test could close its own stream, not that the consumers do). `ClasspathPinotFS.open()` returns a close-tracking `FilterInputStream`, and each test asserts `closedCount == openCount`. One note: the counter is idempotent per stream, because protobuf parsing closes the stream it consumes and the consumer try-with-resources then closes it again - counting raw `close()` calls gave 2 for 1 stream. The assertion is "every opened stream was closed", which is what matters here. -- 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]
