-
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-16173.Improve CopyCommands#Put#executor queue configurability. #3302
Conversation
0d05d96
to
cb5b634
Compare
There are some exceptions in CI/CD: |
@aajisaka , can you help review the code I submitted. |
@jojochuang, @virajjasani, can you help review the code. |
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.
Need to update the newly added param in FileSystemShell.md
as well
@ayushtkn , thanks for your comment. I will continue to improve. |
cb5b634
to
9870dda
Compare
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.
+1 (non-binding), thanks @jianghuazhu
Thanks @virajjasani for the comment. |
@jianghuazhu Thanks for contribution! @virajjasani Thanks for review! |
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/shell/CopyCommands.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/site/markdown/FileSystemShell.md
Outdated
Show resolved
Hide resolved
9870dda
to
c438774
Compare
if (parsedValue < 1) { | ||
threadPoolQueueSize = 1024; |
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.
Better to add a warn log line here, to denote the thread pool queue size was misconfigured and we are now using the default.
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 @ayushtkn for the comment.
I will make some changes later.
c438774
to
9a4658a
Compare
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
9a4658a
to
7abf2a2
Compare
💔 -1 overall
This message was automatically generated. |
Some exceptions occurred here, related to UT: |
hmm, @jojochuang are you aware of some thread/jira disabling it? |
@ayushtkn , thanks for your help. |
@jianghuazhu could you help verify TestCLI isn't broken by this change? We'll have to verify it manually. Thanks. |
Thanks @jojochuang for the comment. I will continue to work. |
} | ||
|
||
@VisibleForTesting | ||
public int getThreadPoolQueueSize() { |
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 method can be package-private
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 @aajisaka for the suggestion, I will continue to work.
7abf2a2
to
80bb9d8
Compare
🎊 +1 overall
This message was automatically generated. |
@jojochuang , I'm very sorry, I previously overlooked a hidden detail in testConf.xml. I have solved it now. |
+1 we're good to go. Thanks @jianghuazhu ! |
…pache#3302) Co-authored-by: zhujianghua <zhujianghua@zhujianghuadeMacBook-Pro.local> Reviewed-by: Hui Fei <ferhui@apache.org> Reviewed-by: Viraj Jasani <vjasani@apache.org>
JIRA: HDFS-16173
NOTICE
Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HADOOP-XXXXX. Fix a typo in YYY.)
For more details, please see https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute