Eager file unmapping in IndexIO, IndexMerger and IndexMergerV9#3422
Eager file unmapping in IndexIO, IndexMerger and IndexMergerV9#3422drcrallen merged 4 commits intoapache:masterfrom
Conversation
…xact purpose for this change is to allow running IndexMergeBenchmark in Windows, however should also be universally 'better' than non-deterministic unmapping, done when MappedByteBuffers are garbage-collected (BACKEND-312)
|
To be complete this should also include metamx/java-util#50, i. e. changing |
| log.info("Skipped files[%s]", skippedFiles); | ||
|
|
||
| v9Smoosher.close(); | ||
| } finally { |
There was a problem hiding this comment.
Closer docs suggest a different pattern here, see https://google.github.io/guava/releases/19.0/api/docs/com/google/common/io/Closer.html for the throwables catching.
leventov
changed the title
|
@drcrallen could you please merge this PR without change in java-util? It passes tests in CI and locally in Linux. |
|
@leventov for some reason there are odd formatting changes with newlines before |
|
@drcrallen done |
|
Cool 👍 after TravisCI Still needs another +1 from another committer. |
|
Hmm, the general idea looks good to me, although if we're going down the road of being more unmappy-happy with ByteBuffers, I think it'd be nice to use a function that does a Files.map and then wraps the result in a ResourceHolder. That way they can be used with try-with-resources, which is cleaner than using Closers imo. Also they could be composed into already existing things we have like ReferenceCountingResourceHolder, if we have some need for refcounted mmapped files. |
|
👍 with or without the ResourceHolder-returning mmap function. But I do think that would be nice if we are going down this road. |
|
I propose accepting the Closer approach for now and @leventov sending a notice out to the dev mailing list about the potential future need for a more advanced and prettier try-with-resources friendly class with a callout to this PR. So that in the future if the use gets expanded, it will be time to consider the use cases and add a proper class (with proper test framework support) |
|
I'm 👍 on accepting the Closer stuff for this PR. IMO a dev notice isn't super necessary but wouldn't hurt. |
|
@drcrallen @gianm I'm on board with that as well |
|
@drcrallen @gianm was there a CLA signed? |
|
@fjy he's covered under MMX corporate |
4 participants
The exact purpose for this change is to allow running IndexMergeBenchmark in Windows, however should also be universally 'better' than non-deterministic unmapping, done when MappedByteBuffers are garbage-collected