HADOOP-13327. Specify Output Stream and Syncable #2102
Closed
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
|
hi @busbey @DadanielZ -this is the rebased version of #1694. Could you two take a look at it and see if we can get it in. Not just because it says what we expect from hsync/hflush, but because we add the ability to probe a stream for having it before actually attempting the call. All output streams in our codebase which implement Syncable do the right thing here |
|
rawlocal FS failure is because even though localfs.xml says hsync is supported, it's not being passed all the way down. I need a subclass of java.io.ByteBufferOutputStream to pass it through. The good news, that comes in #2069; I'm going to change the xml test declarations until that is in |
joshelser
reviewed
Jun 29, 2020
...-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/StoreImplementationUtils.java
Outdated
Show resolved
Hide resolved
...-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/StoreImplementationUtils.java
Show resolved
Hide resolved
| This MAY be a bug, as it allows >1 client to create a file with `overwrite==false`, | ||
| and potentially confuse file/directory logic | ||
| This is a significant difference between the behavior of object stores | ||
| and that of filesystems, as it allows >1 client to create a file with `overwrite==false`, |
There was a problem hiding this comment.
nit: You used overwrite=false below. I'm assuming the double-equals was just copy-paste, but maybe correct to stay consistent?
There was a problem hiding this comment.
changed to ==; also replace >1 with > to keep everything happy
| model permits the output stream to have `close()` invoked while awaiting an | ||
| acknowledgement from datanode or namenode writes in an `hsync()` operation. | ||
|
|
||
| ### <a name="consistencyy"></a> Consistency and Visibility |
steveloughran
force-pushed
the
filesystem/HADOOP-13327-outputstream-spec-lean
branch
from
28bc9f4 to
9f4fb5c
Compare
|
@joshelser -rebased and addressed your minor points I think you are right, we don't need separate HSYNC/HFLUSH options, it's just that they've been there (separately) for a while. What to do?
I like option 3, now I think about it. |
steveloughran
force-pushed
the
filesystem/HADOOP-13327-outputstream-spec-lean
branch
from
99ad6e0 to
e818e68
Compare
|
TestRawLocalContract create is failing until we do our own BufferedOutputStream in passthrough of HADOOP-16830/ #2323 . the HDFS checksum ones are about checksums not being found, which implies they weren't being written. Which goes near output streams, doesn't it? Which means there is a risk this is a genuine regression and not "hdfs tests being flaky" |
|
Thinking:
|
steveloughran
force-pushed
the
filesystem/HADOOP-13327-outputstream-spec-lean
branch
from
e818e68 to
3757462
Compare
|
HSYNC is the only option needed; tag HFLUSH as deprecated and in the docs say its not useful; outputstream.md doc lists the standard options. |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
force-pushed
the
filesystem/HADOOP-13327-outputstream-spec-lean
branch
from
3757462 to
528739e
Compare
|
💔 -1 overall
This message was automatically generated. |
|
Yikes. These got mis-filed in my mail. I'm sorry, Steve.
My only worry with this is that, today, HBase is only running with HFlush and not HSync because of performance reasons. The last wag I remember was in the ballpark of a 30% performance hit for writes if we force hsync instead of hflush. Let me re-read your latest and see if my mind changes. |
joshelser
reviewed
Nov 18, 2020
| it MAY be durable. That is: it does not have to be persisted, merely guaranteed | ||
| to be consistently visible to all clients attempting to open a new stream reading | ||
| data at the path. | ||
| 1. `Syncable.hsync()` MUST flush the data and persist data to the underlying durable |
There was a problem hiding this comment.
Nit: clarify that "flush" in this sense refers to "hflush" and not "OutputStream.flush".
You clarify in detail earlier that hsync must do all the hflush also does. Just avoiding any opportunity for a reader to have missed that section and skipped to here.
There was a problem hiding this comment.
done. Also added a bit about hflush being able to forward to hsync, but not the other way around
|
My only thought at this point is: are we creating a dichotomy by having HFLUSH be deprecated in The argument is that we would never have an implementor of hflush which doesn't also implement hsync. However, every implementor of hsync should have an implementation for hflush (even if that implementation is to just invoke hsync). Now that I've come to this, I think outputstream.md adequately does this. Do you think it's appropriate to have a reciprocal javadoc reference from This was a fantastic and thorough read. Thanks for taking the time to do it, Steve! |
|
yeah, lets cross ref the javadocs. "if you implement this you SHOULD declare that you support the capability" |
mehakmeet
reviewed
Nov 19, 2020
| @@ -32,7 +32,7 @@ | |||
| import java.util.concurrent.Callable; | |||
| import java.util.concurrent.Future; | |||
| import java.util.concurrent.TimeUnit; | |||
|
|
|||
| This | |||
There was a problem hiding this comment.
"This" should be removed. This might be the reason why Yetus is failing.
There was a problem hiding this comment.
heh, that's what comes of having speech recognition turned on
hadoop-common-project/hadoop-common/src/site/markdown/filesystem/outputstream.md
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/site/markdown/filesystem/outputstream.md
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/site/markdown/filesystem/outputstream.md
Outdated
Show resolved
Hide resolved
| writing small 4KB blocks or similar. | ||
|
|
||
| Forwarding this to a full flush across a distributed filesystem, or worse, | ||
| a distant object store, is very underperformant |
There was a problem hiding this comment.
nit: typo in "underperforming"
There was a problem hiding this comment.
It is actually a word, AFAIK. But clearly too pretentious. Will fix
|
|
||
| 1. The check for existing data in a `create()` call with `overwrite=False`, may | ||
| take place in the `create()` call itself, in the `close()` call prior to/during | ||
| the write, or at some point in between. Expect in the special case that the |
steveloughran
added
enhancement
fs
fs/azure
steveloughran
added
the
fs/s3
joshelser
approved these changes
Nov 19, 2020
This comment has been minimized.
This comment has been minimized.
|
wow, that was a big failure. lets rebase and see what can "go away"., then look @ failures in detail |
This PR removes the changes related to S3A output stream lifecycle, so only covers the specification of Syncable and ensures that StreamCapabilities passes all the way through to the final implementation classes. All streams which implement Syncable hsync/hflush declare this in their stream capabilities Change-Id: I82b16a8e0965f34eb0c42504da43e8fbeabcb68c
Change-Id: Id38cf27639215abdd0d8c3578ddf72ed7adca8c5
TODO "Could this be in a section about visibility and not in the model definition? Maybe later. "here's the model, here's how that model works with creation, here's how it works when reading/writing" flows much better and visibility should be in that third part." Change-Id: I61c89475a1ea72006524803f2a7dd9e40551d718
Review with more on 404 caching. Change-Id: Ib474a84e48556c6b76121427a026fa854b5bd9e0
Doesn't actually get through as there's a BufferedOutputStream in the way... will need to do something there :) Change-Id: Ib2e4517e266168f3ad106e55ceac987ed443aeec
Change-Id: I95e6b63eda051afae22a9d48c35b5e69e0464852
* use (renamed) predicate isProbeForSyncable() everywhere appropriate * HFLUSH marked as deprecated * outputstream.md lists standard capabilities, declares HFLUSH as deprecated. * clean up tests probing for the capabilities Change-Id: I2a8c2ddc7119c31a73ee327a181bf700a2c5f21f
Change-Id: I00df6431167e04f2210d76d5712d8c16e70f1c70
steveloughran
force-pushed
the
filesystem/HADOOP-13327-outputstream-spec-lean
branch
from
8c139f4 to
e7a1212
Compare
This comment has been minimized.
This comment has been minimized.
|
Closing as #2587 is a rebased successor, one where the use of the new |
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.
successor to #1694