-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
HDFS-6874. Add GETFILEBLOCKLOCATIONS operation to HttpFS #4750
HDFS-6874. Add GETFILEBLOCKLOCATIONS operation to HttpFS #4750
Conversation
🎊 +1 overall
This message was automatically generated. |
@amahussein @jojochuang - Can you please help in reviewing this PR? Thanks. |
@ferhui - Can you please help in reviewing this. Thanks. |
@ashutoshcipher Thanks for involving me. Will look into it when I am available. One or two days later. |
@ferhui Thank you so much. Please take your time. |
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.
Thanks @ashutoshcipher for your good job. Left some comments, please fix them. Thanks~
...-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSServer.java
Outdated
Show resolved
Hide resolved
...-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSServer.java
Show resolved
Hide resolved
...oop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSParametersProvider.java
Outdated
Show resolved
Hide resolved
...-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/FSOperations.java
Outdated
Show resolved
Hide resolved
...-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/FSOperations.java
Outdated
Show resolved
Hide resolved
...ject/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServer.java
Outdated
Show resolved
Hide resolved
...ject/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServer.java
Outdated
Show resolved
Hide resolved
...ject/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServer.java
Show resolved
Hide resolved
...op-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestJsonUtilClient.java
Outdated
Show resolved
Hide resolved
...op-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/web/TestJsonUtilClient.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@ferhui - Please help with review. 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.
Thanks @ashutoshcipher for your reminder and sorry for the later response.
Leave some minor comments.
BTW, the new checkstyle has increased the maximum number of characters on a single line from 80 to 100, so some modifications can be moved to one single line.
...-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSServer.java
Outdated
Show resolved
Hide resolved
long length = ((Number) m.get("length")).longValue(); | ||
long offset = ((Number) m.get("offset")).longValue(); | ||
boolean corrupt = Boolean.getBoolean(m.get("corrupt").toString()); | ||
String[] storageIds = toStringArray(getList(m, "storageIds")); |
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.
If you want use this storageIds
, why not modify the serialize code to add it? here
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.
Thanks for suggestion. I think current way looks fine too. Any specific reason to modify?
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 just think that it's an invalidate code line. Because the serialized code didn't serialize it. Not block this PR.
...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/web/WebHdfsFileSystem.java
Outdated
Show resolved
Hide resolved
} | ||
} catch (RemoteException e) { | ||
// parsing the exception is needed only if the client thinks the service is compatible | ||
if (isServerHCFSCompatible && isGetFileBlockLocationsException(e)) { |
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 add one UT to test this fallback?
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.
Sure
...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/web/WebHdfsFileSystem.java
Outdated
Show resolved
Hide resolved
import org.eclipse.jetty.webapp.WebAppContext; | ||
|
||
import java.io.File; | ||
import java.io.FileOutputStream; |
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.
Rollback this modification?
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 just import optimization. may be we can keep this. What do you think?
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.
It's ok. But I like to minimize modification.
...ct/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/client/BaseTestHttpFSWith.java
Outdated
Show resolved
Hide resolved
if (!this.isLocalFS()) { | ||
FileSystem fs = this.getHttpFSFileSystem(); | ||
testFile = new Path(getProxiedFSTestDir(), "singleBlock.txt"); | ||
DFSTestUtil.createFile(fs, testFile, (long) 1, (short) 1, 0L); |
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 remove the redundant cast (long)
.
...ject/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServer.java
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/web/JsonUtil.java
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Is passing in local. Will trigger jenkins once again after getting current code being reviewed |
Hi @ZanderXu - Can you please help in reviewing ? Thank you so much. |
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.
LGTM. Thanks @ashutoshcipher .
long length = ((Number) m.get("length")).longValue(); | ||
long offset = ((Number) m.get("offset")).longValue(); | ||
boolean corrupt = Boolean.getBoolean(m.get("corrupt").toString()); | ||
String[] storageIds = toStringArray(getList(m, "storageIds")); |
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 just think that it's an invalidate code line. Because the serialized code didn't serialize it. Not block this PR.
import org.eclipse.jetty.webapp.WebAppContext; | ||
|
||
import java.io.File; | ||
import java.io.FileOutputStream; |
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.
It's ok. But I like to minimize modification.
Thanks @ZanderXu for review.
Thanks for mentioning not being a blocker in this PR.
Reverted the import changes in my latest commit. |
Thanks @ZanderXu for your review and final approval :) |
💔 -1 overall
This message was automatically generated. |
Thanks @ZanderXu for confirming. I have created JIRA for it - HDFS-16801. Please feel to take it over incase you find the root cause. Meanwhile I will also try looking into it. Thanks. |
Many congratulations @ZanderXu for committing a committer 🎉 Well deserved and great work. 😃 |
@ZanderXu - Can we commit/merge this? |
Merged into trunk. Thanks @ashutoshcipher for your contribution. |
Co-authored-by: Ashutosh Gupta <ashugpt@amazon.com>
Description of PR
Add GETFILEBLOCKLOCATIONS operation to HttpFS.
How was this patch tested?
UT
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?