HADOOP-16785: wasb to raise IOE if write() invoked on a closed stream #1791
Conversation
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
with * test * bit of extra asserts to help debug config * and LTU.intercept to declare class of expected exception when none was raised. Change-Id: I37951f0a3881d9b9849c4a51159d6670583f65c1
|
Tested: azure wales @bgaborg , @ThomasMarquardt -fancy a quick look at this? |
|
🎊 +1 overall
This message was automatically generated. |
It is, but hsync() fails on a closed file. Commented out that operation. It does handle flush(), which is good as some apps do call it on closed files. Change-Id: I25414cc500feb9391dc520e5292250945d6576e6
Change-Id: I26af214bd40b28aa006d27e6ef22b8cefb39a799
|
I can't replicate that in the test -but can see how it does surface...it's in the close() of a try-with-resources
I'm patching flushInternal() so that if it rethrows an exception in close, it wraps that with a covering IOE. (it still nests the inner, so there is a loop in the chain). |
Change-Id: I5ffa417219249e102bf7e861d357b57af3b780f0
|
tested -azure wales. All good except for HADOOP-16706 and org.apache.hadoop.fs.azurebfs.ITestClientUrlScheme |
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
| throw new IOException("Skipping final flush and write due to " + e, e); | ||
| } else | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
Instead, I think we should update AbfsOutputStream.close to wrap the exception, short of having the Java implementers fix this. :)
There was a problem hiding this comment.
thought of that, but also thought it might be good to differentiate exception raised in the later bits of the flush. Keeping it in close() is cleaner as it puts the problem where it belongs
...hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCreate.java
Show resolved
Hide resolved
| * exception rethrows the first. | ||
| */ | ||
| @Test | ||
| public void testFilteredDoubleClose() throws Throwable { |
There was a problem hiding this comment.
Double close would have already been passing with these changes. To really test the try-with-resources I think you need to modify the underlying blob after fs.create but before flush so that flush fails and sets AbfsOutputStream.lastError. For example, if you write some data to the blob so that the position held by the AbfsOutputStream is invalid, flush may fail. For example, the following test:
out = fs.create(path)
out.write('a');
// externally write data to the same path so that the next call to flush will fail due to position being off.
intercept(IOException.class, () -> out.flush());
intercept(IOException.class, () -> out.close());
There was a problem hiding this comment.
You are right, I couldn't think how to reliably replicate it. without going near mockito etc.,
or we add a package level setLastError() call?
There was a problem hiding this comment.
I think you can simply create a second stream for the same path, or even call delete on it after writing some data with the other stream. Basically, write to the same path with two streams.
There was a problem hiding this comment.
yes, write+delete does the trick
|
checkstyle |
of the same class. This moves the wrapping logic out of flushInternal into close(), using IOUtils.wrapException to ensure the type of the wrapped exception is the same as that caught (without that the E2E exceptions fail). the test uses try-with-resources as suggested by Thomas; we delete a file while it is being written; this causes hsync to fail -with close() also throwing an exception which is then caught and suppressed. Change-Id: If1bcd2885bccbf39de888b8295a5a7e529935f2d
|
Updated patch adds the test recommended by thomas and wraps the exception in close, while retaining the class of the original exception. Tested: ABFS wales. It's very fast :)
|
|
🎊 +1 overall
This message was automatically generated. |
ThomasMarquardt
approved these changes
Jan 7, 2020
|
added a followup #1809 -an extra test to confirm the filter output stream close was happy |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
adds
was raised.
Change-Id: I37951f0a3881d9b9849c4a51159d6670583f65c1