kaori-seasons commented on code in PR #6773: URL: https://github.com/apache/paimon/pull/6773#discussion_r2601274924
########## paimon-lucene/src/main/java/org/apache/paimon/lucene/index/IndexMMapDirectory.java: ########## @@ -0,0 +1,56 @@ +/* + * 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.paimon.lucene.index; + +import org.apache.lucene.store.MMapDirectory; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.UUID; + +/** A wrapper of MMapDirectory for vector index. */ +public class IndexMMapDirectory implements AutoCloseable { + private final Path path; + private final MMapDirectory mmapDirectory; + + public IndexMMapDirectory() throws IOException { + this.path = java.nio.file.Files.createTempDirectory("paimon-lucene-" + UUID.randomUUID()); + this.mmapDirectory = new MMapDirectory(path); + } + + public MMapDirectory directory() { + return mmapDirectory; + } + + public void close() throws Exception { Review Comment: Current situation: IndexMMapDirectory creates a temporary directory. `close` calls `mmapDirectory.close()` and then `walk` to delete it. However: On Windows, the mmap file might be occupied and cannot be deleted, and `close` might not completely release the mapping (explicit unmap is usually not possible at the JVM level). The `close` method signature throws an `Exception(AutoCloseable)` and swallows the deletion exception (ignoring cleanup errors), without warnings or rollbacks. Recommendation: Throw an `IOException` (or at least log/throw) with `close`, instead of swallowing the exception completely. As a fallback when deletion fails: call `Files.walk(...).forEach(p -> p.toFile().deleteOnExit())`. This guarantees cleanup at process termination (although not ideal). On Windows platforms, you can fall back to using FSDirectory (not mmap) or retry several times and wait a short time if deletion fails. Create the temporary directory in a more controllable parent directory or allow injection of a base temp dir (for easier testing and cleanup). example ``` public class IndexMMapDirectory implements AutoCloseable { private final Path path; private final MMapDirectory mmapDirectory; public IndexMMapDirectory() throws IOException { this.path = Files.createTempDirectory("paimon-lucene-" + UUID.randomUUID()); this.mmapDirectory = new MMapDirectory(path); this.path.toFile().deleteOnExit(); // best-effort } public MMapDirectory directory() { return mmapDirectory; } @Override public void close() throws IOException { IOException firstEx = null; try { mmapDirectory.close(); } catch (IOException e) { firstEx = e; } // attempt to delete files; if fails, mark deleteOnExit try { if (Files.exists(path)) { Files.walk(path) .sorted(Comparator.reverseOrder()) .forEach(p -> { try { Files.delete(p); } catch (IOException e) { // fallback: schedule delete on exit p.toFile().deleteOnExit(); } }); } } catch (IOException e) { if (firstEx == null) { firstEx = e; } else { firstEx.addSuppressed(e); } } if (firstEx != null) { throw firstEx; } } } ``` -- 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]
