-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HDFS-14564: Add libhdfs APIs for readFully; add readFully to ByteBufferPositionedReadable #963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Thanks for the patch @sahilTakiar . Well-written test case. Looks good to me. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
+1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception message could be confusing; it's not the CryptoInputStream that doesn't support byte buffer positioned reads, it's the input stream wrapped by it. How about printing the class name of the object in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what this means. Does it mean the implementation of this interface must guanrantee thread safety despite the fact that not all file systems provide intrinsic thread-safety?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally copied this from PositionedReadable, but after taking a second look, the only two filesystems that implement this interface are CryptoInputStream and DFSInputStream and both of their implementations are thread safe. So I removed this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, would be nice to log the class name of the input stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me. Just a few minor comments. Other than that I am +1 too
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
6f976b1 to
11fdcb6
Compare
|
💔 -1 overall
This message was automatically generated. |
…erPositionedReadable
11fdcb6 to
e5c7553
Compare
|
💔 -1 overall
This message was automatically generated. |
|
@jojochuang @smengcl any other comments? |
|
Sorry, I missed to update. +1 |
|
Looks good. Thanks for the work @sahilTakiar . Thanks for committing @jojochuang ! |
…erPositionedReadable (apache#963) Contributed by Sahil Takiar. Reviewed-by: Siyao Meng <[email protected]> (cherry picked from commit 13b427f) Change-Id: I66ecf3a4c0657ca2129e3ab5ec04739c0f096903
prateekm jagadish-v0 please take a look. Would be nice to deploy this before the meetup talking about Samza 1.0 🙂 Author: Daniel Chen <[email protected]> Reviewers: Jagadish <[email protected]> Closes apache#963 from dxichen/update-website-release-meetups
…erPositionedReadable (apache#963) Contributed by Sahil Takiar. Reviewed-by: Siyao Meng <[email protected]>
…erPositionedReadable (apache#963) Contributed by Sahil Takiar. Reviewed-by: Siyao Meng <[email protected]>
…erPositionedReadable (apache#963) Contributed by Sahil Takiar. Reviewed-by: Siyao Meng <[email protected]>
…erPositionedReadable (apache#963) Contributed by Sahil Takiar. Reviewed-by: Siyao Meng <[email protected]>
…erPositionedReadable (apache#963) Contributed by Sahil Takiar. Reviewed-by: Siyao Meng <[email protected]>
…erPositionedReadable (apache#963) Contributed by Sahil Takiar. Reviewed-by: Siyao Meng <[email protected]>
…erPositionedReadable (apache#963) Contributed by Sahil Takiar. Reviewed-by: Siyao Meng <[email protected]> asd
…erPositionedReadable (apache#963) Contributed by Sahil Takiar. Reviewed-by: Siyao Meng <[email protected]>
…erPositionedReadable (apache#963) Contributed by Sahil Takiar. Reviewed-by: Siyao Meng <[email protected]>
…erPositionedReadable (apache#963) Contributed by Sahil Takiar. Reviewed-by: Siyao Meng <[email protected]>
…erPositionedReadable (apache#963) Contributed by Sahil Takiar. Reviewed-by: Siyao Meng <[email protected]>
…ByteBufferPositionedReadable (apache#963) Contributed by Sahil Takiar." This reverts commit 12d7d26.
…ByteBufferPositionedReadable (apache#963) Contributed by Sahil Takiar." This reverts commit 12d7d26.
HDFS-14564: Add libhdfs APIs for readFully; add readFully to ByteBufferPositionedReadable
readFullytoByteBufferPositionedReadableand exposes it via libhdfsPositionedReadable#readFullyvia libhdfshdfsPreadandhdfsReadif the underlying stream supportsByteBufferreads, theByteBufferAPIs will be used