Minor fixes for MergeOnRead MVP release readiness #387
Conversation
This file contains 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
Contributor
n3nash
commented
May 1, 2018
•
n3nash
commented
- Changes to be able to pass basePath for spillable file from config
- Handling reading of archival files issue when no instant time is present (as is the case with archiving)
- Exception handling if unable to create an outputstream from append()
n3nash
force-pushed
the
minor_fixes_for_mor_mvp
branch
from
82cbffa to
c30778e
Compare
n3nash
force-pushed
the
minor_fixes_for_mor_mvp
branch
from
c30778e to
d3660e4
Compare
n3nash
force-pushed
the
minor_fixes_for_mor_mvp
branch
2 times, most recently
from
5a16677 to
5d60e14
Compare
vinothchandar
approved these changes
May 7, 2018
| @@ -205,6 +210,9 @@ private void flush() throws IOException { | |||
| } | |||
| output.flush(); | |||
| output.hflush(); | |||
| // NOTE : the following API call makes sure that the data is flushed to disk on DataNodes (akin to POSIX fsync()) | |||
| // See more details here : https://issues.apache.org/jira/browse/HDFS-744 | |||
| output.hsync(); | |||
There was a problem hiding this comment.
does nt hsync() call hflush() ? Can we check on this. Could save 1 RPC
There was a problem hiding this comment.
Looks like it : "Similar to posix fsync, flush out the data in client's user buffer all the way to the disk device (but the disk may have it in its cache)"
Removed hflush().
| && fs instanceof DistributedFileSystem) { | ||
| // data node is going down. Note that we can only try to recover lease for a DistributedFileSystem. | ||
| // ViewFileSystem unfortunately does not support this operation | ||
| if ((e.getClassName().contentEquals(AlreadyBeingCreatedException.class.getName()) || e.getClassName() |
There was a problem hiding this comment.
suggest pull these conditions into a method named after the check its performing, for ease of reading.
There was a problem hiding this comment.
moved the recoverlease into a new method, not moving append handling since the next PR (#388) will make that code neater.
n3nash
force-pushed
the
minor_fixes_for_mor_mvp
branch
from
5d60e14 to
f66a2c0
Compare
|
@vinothchandar addressed comments. |
|
@n3nash can you please rebase and push again |
n3nash
force-pushed
the
minor_fixes_for_mor_mvp
branch
from
f66a2c0 to
79f601f
Compare
|
@vinothchandar done. |
n3nash
force-pushed
the
minor_fixes_for_mor_mvp
branch
from
79f601f to
eb829d0
Compare
n3nash
force-pushed
the
minor_fixes_for_mor_mvp
branch
from
eb829d0 to
5bcb142
Compare
# for free
to join this conversation on GitHub.
Already have an account?
# 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.