-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-27746 Check if the file system supports storage policy before invoking setStoragePolicy() #5189
Conversation
…nvoking setStoragePolicy() Change-Id: I1fcc90ec23f3b23a6876f094436320d9eb8480ee
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Please run mvn spotless to fix the style issue? |
Change-Id: I7cab20073e24b97dd6b3090723f30941febd40b4
Thanks for the tips! Updated checkstyle and also fixed the test. |
FileSystem testFs = new AlwaysFailSetStoragePolicyFileSystem(); | ||
testFs.initialize(new URI("hdfs://localhost/"), conf); |
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 line is required otherwise uri is null pointer and will fail hasPathCapability() API.
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.
Can we not use the 'hdfs' scheme here? It is not HDFS, actually...
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
The failed test doesn't look related. @taklwu can you help review this one? |
The only concern is about the scheme we use when testing, all others are OK. |
Any updates here @jojochuang ? |
Well actually the test would fail if it's not "hdfs"
|
Looks like this one got missed out. I'll merge it now. |
Hi @jojochuang we should put this in branch-3 as well, right? Do you plan to backport? |
Yes thanks for reminder. I'll take this up. |
…nvoking setStoragePolicy() (apache#5189) (cherry picked from commit 0646151)
…nvoking setStoragePolicy() (apache#5189) (cherry picked from commit 0646151) (cherry picked from commit 35b11eb)
…nvoking setStoragePolicy() (apache#5189) (cherry picked from commit 0646151) (cherry picked from commit 35b11eb) (cherry picked from commit d93c0c0)
Simply check and ignore silently if the underlying FS does not support storage policy.
the hasPathCapability() API is Hadoop 3.2 and above. So do not cherrypick this change to HBase 2.x or lower.