Copy Lucene IOUtils #29012
Copy Lucene IOUtils #29012
Conversation
As we have factored Elasticsearch into smaller libraries, we have ended up in a situation that some of the dependencies of Elasticsearch are not available to code that depends on these smaller libraries but not server Elasticsearch. This is a good thing, this was one of the goals of separating Elasticsearch into smaller libraries, to shed some of the dependencies from other components of the system. However, this now means that simple utility methods from Lucene that we rely on are no longer available everywhere. This commit copies IOUtils (with some small formatting changes for our codebase) into the fold so that other components of the system can rely on these methods where they no longer depend on Lucene.
jasontedor
added
>non-issue
review
:Core/Infra/Core
|
Pinging @elastic/es-core-infra |
| } | ||
| } | ||
|
|
||
| public void testRm() throws IOException { |
There was a problem hiding this comment.
I am not sure how to test the exceptional case of IOUtils#rm, it seems difficult to force an I/O exception while deleting a file; I am open to suggestions or we leave it uncovered (as the Lucene version is uncovered too).
There was a problem hiding this comment.
Lucene has a FilterFileSystemProvider which allows mocking of various file system calls (for example the VirusCheckingFS class in the lucene test-framework which throws AccessDeniedExceptions on delete)
There was a problem hiding this comment.
Thanks @romseygeek; I pushed a test in 523e36b.
| */ | ||
| public static void closeWhileHandlingException(final Iterable<? extends Closeable> objects) { | ||
| for (final Closeable object : objects) { | ||
| // noinspection EmptyCatchBlock |
There was a problem hiding this comment.
Is this a thing in our codebase?
There was a problem hiding this comment.
It keeps IntelliJ from complaining about the empty catch block.
| object.close(); | ||
| } | ||
| } catch (final Throwable t) { | ||
|
|
There was a problem hiding this comment.
Should we log something here? It has always bothered me that lucene eats these exceptions.
There was a problem hiding this comment.
If we don't want to diverge from lucene I understand that though.
There was a problem hiding this comment.
But if that is the case we should leave a big comment about it.
There was a problem hiding this comment.
We do not have a logger right now. Let us consider this in a follow-up, this is a broader problem that we might want to do something about (SLF4J 🙈).
| return unremoved; | ||
| } | ||
|
|
||
| // TODO: replace with constants class if needed (cf. org.apache.lucene.util.Constants) |
There was a problem hiding this comment.
This comment is probably not useful to us.
There was a problem hiding this comment.
I put this comment here 😐. In Lucene, these use org.apache.lucene.util.Constants which we do not have access to here. I did not want to wholesale copy right now (and would like to avoid if reasonable).
| o.write("0\n".getBytes(StandardCharsets.US_ASCII)); | ||
| } | ||
| IOUtils.fsync(file, false); | ||
| // no exception |
There was a problem hiding this comment.
This seems like as super critical thing not to have a stronger assertion about. I think it is worth brainstorming a better test for a followup.
As we have factored Elasticsearch into smaller libraries, we have ended up in a situation that some of the dependencies of Elasticsearch are not available to code that depends on these smaller libraries but not server Elasticsearch. This is a good thing, this was one of the goals of separating Elasticsearch into smaller libraries, to shed some of the dependencies from other components of the system. However, this now means that simple utility methods from Lucene that we rely on are no longer available everywhere. This commit copies IOUtils (with some small formatting changes for our codebase) into the fold so that other components of the system can rely on these methods where they no longer depend on Lucene.
5 participants
As we have factored Elasticsearch into smaller libraries, we have ended up in a situation that some of the dependencies of Elasticsearch are not available to code that depends on these smaller libraries but not server Elasticsearch. This is a good thing, this was one of the goals of separating Elasticsearch into smaller libraries, to shed some of the dependencies from other components of the system. However, this now means that simple utility methods from Lucene that we rely on are no longer available everywhere. This commit copies IOUtils (with some small formatting changes for our codebase) into the fold so that other components of the system can rely on these methods where they no longer depend on Lucene.