Remove Assertion on fsync Dir IOE#42883
Closed
original-brownbear wants to merge 1 commit intoelastic:masterfrom
original-brownbear:remove-fsync-assertion
Closed
Remove Assertion on fsync Dir IOE#42883original-brownbear wants to merge 1 commit intoelastic:masterfrom original-brownbear:remove-fsync-assertion
original-brownbear wants to merge 1 commit intoelastic:masterfrom
original-brownbear:remove-fsync-assertion
Conversation
* This assertion is not correct. `fsync` might not throw on OSX and Linux, but you also get this IOException if you try to open a directory whose parent does not exist for example (or whatever other IOException you might get from opening a directory).
Collaborator
|
Pinging @elastic/es-core-infra |
jasontedor
requested changes
Jun 6, 2019
Member
jasontedor
left a comment
There was a problem hiding this comment.
I don't think this is the right change. Rather, I think that we want to move this logic to be inside the try-with-resources block, so that we aren't being lenient on failing to fsync the directory if we, for example, fail to open the directory because it does not exist or the disk is crap. Today, we are lenient in that regard and that looks dangerous to me. So rather, I think we should change along the lines of:
diff --git a/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java b/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java
index 46d19d2a814..f82070183e0 100644
--- a/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java
+++ b/libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java
@@ -264,17 +264,17 @@ public final class IOUtils {
*/
public static void fsync(final Path fileToSync, final boolean isDir) throws IOException {
try (FileChannel file = FileChannel.open(fileToSync, isDir ? StandardOpenOption.READ : StandardOpenOption.WRITE)) {
- file.force(true);
- } catch (final IOException ioe) {
- if (isDir) {
- assert (LINUX || MAC_OS_X) == false :
- "on Linux and MacOSX fsyncing a directory should not throw IOException, "+
- "we just don't want to rely on that in production (undocumented); got: " + ioe;
- // ignore exception if it is a directory
- return;
+ try {
+ file.force(true);
+ } catch (final IOException e) {
+ if (isDir) {
+ assert (LINUX || MAC_OS_X) == false :
+ "on Linux and MacOSX fsyncing a directory should not throw IOException, "+
+ "we just don't want to rely on that in production (undocumented); got: " + e;
+ return;
+ }
+ throw e;
}
- // throw original exception
- throw ioe;
}
}
}Then, I'm not sure that we need to drop the assertion.
Member
Contributor
Author
|
@jasontedor as I said in your PR, I agree your solution makes more sense. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fsyncmight not throw on OSX and Linux, but you also get this IOException if you try to open a directory whose parent does not exist for example (or whatever other IOException you might get from opening a directory).I think we also have the same assertion in Lucene, I'd open a PR for that too if this one is deemed ok :)