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.
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
HADOOP-19139.No GetPathStatus for opening AbfsInputStream #6699
base: trunk
Are you sure you want to change the base?
HADOOP-19139.No GetPathStatus for opening AbfsInputStream #6699
Changes from 67 commits
e2a4e05
8d4756b
4aae0c8
2d6f0cb
5939e60
c8a6a00
4ad349c
ee1f4c3
cfc4f8e
cc09403
6cb1773
bd920a5
37bc691
7ecabf9
2c9da9e
58a399c
7d22917
52e19c0
8c76674
a06a2a1
8f44c96
b66eea6
45a0796
b1fd443
38f5592
074fb39
99e397c
fe1df56
0475b3e
a1c56c2
dc898c8
9e7bb6c
cb929f9
e7b121a
51373f0
9b998d4
7d1a274
cd9b0de
0c454d7
5c3553b
b224fa2
2fc5bbd
e7cfec5
2f51126
02b3421
4c08320
f62f868
36ba7c0
7978c27
8d38aed
811693d
065919a
9e73700
1b3ba1b
f4fef33
caf6c56
480a705
e7cd9a3
4154485
caa0756
5ddf14a
0b53e1e
bd86173
44ffeb3
01239aa
ffefdb3
20f4f2b
0a1b23c
19c1d94
429a518
d096f3d
9231959
4547f53
4df6b02
c8299fc
6de6b88
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This comment should be updated
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.
Here, why do we need EncryptionType.ENCRYPTION_CONTEXT in the condition?
Should the condition be like below
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.
Good point. Actually, in case the encryptionType is ENCRYPTION_CONTEXT, read API needs to have following request headers:
To create these headers, encryptionContext is required which is given by the server on getPathStatus.
Hence, in case of ENCRYPTION_CONTEXT, we need to get the getPathStatus.
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.
Presently, say abfsConfiguration.isInputStreamLazyOptimization is disabled then it will enter into the block even if its not ENCRYPTION_CONTEXT ?
Appreciates if you could clarify - on what condition the below code should be skipped(can't be executed) ?
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 completely understand that logic has become bit messy and not easy to read. Have refactored the logic. Hope the new logic is more readable and understandable:
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.
Its good to keep the conditions simple for the code maintenance. Have you added
filestatus != null.
for the defensive coding?If
filestatus != null
check is really required, then please add java comments about the case where it becomes null.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.
Agreed, its not required. Have removed it. Thanks!
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.
Should we name this as per format followed by other HTTP Status code constants:
HTTP_RANGE_NOT_SATISFIABLE
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.
416 is not given constant in HttpUrlConnection class. Hence, maintaining a new constant in AbfsHttpConstants
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.
That makes sense. But what I was proposing is to name this new variable as per the format followed by HttpUrlConnection class.
What are your thoughts??
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.
use
StringUtils.isNotEmpty
for better readability.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.
Make sense. Taken.