-
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
HDDS-1879. Support multiple excluded scopes when choosing datanodes in NetworkTopology #1194
Conversation
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
/retest |
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/InnerNodeImpl.java
Show resolved
Hide resolved
public static String removeDuplicate(NetworkTopology topology, | ||
Collection<Node> mutableExcludedNodes, String excludedScope, | ||
public static void removeDuplicate(NetworkTopology topology, | ||
Collection<Node> mutableExcludedNodes, List<String> mutableExcludedScopes, | ||
int ancestorGen) { | ||
if (mutableExcludedNodes == null || mutableExcludedNodes.size() == 0 || |
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.
NIT: We can use CollectionUtils.isEmpty() to simplify the code at 79/80.
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.
OK
@@ -176,7 +176,7 @@ Node chooseRandom(String scope, Collection<Node> excludedNodes, | |||
* | |||
* @return the chosen node | |||
*/ | |||
Node chooseRandom(String scope, String excludedScope, | |||
Node chooseRandom(String scope, List<String> excludedScopes, |
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 don't see this being used anywhere, should we remove it?
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.
OK
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/NetworkTopologyImpl.java
Show resolved
Hide resolved
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/NetworkTopologyImpl.java
Show resolved
Hide resolved
throw new IllegalArgumentException("excludedScope " + excludedScope + | ||
" cannot start with " + SCOPE_REVERSE_STR); | ||
private void checkExcludedScopes(List<String> excludedScopes) { | ||
if (excludedScopes != null && excludedScopes.size() > 0) { |
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.
NIT: use CollectionUtils.isEmpty() to simplify?
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 @ChenSammi for the patch. It looks good to me overall. Just few minor comments inline..
💔 -1 overall
This message was automatically generated. |
…n NetworkTopology.
2256776
to
3debbb8
Compare
💔 -1 overall
This message was automatically generated. |
Thanks @ChenSammi for updating the PR. The latest change LGTM. Can you fix the checkstyle and unit test failure that seems to be related? +1 after that. |
Thanks @xiaoyuyao for the review. A new patch to fix the check style issue. The failed UTs are not related. Will commit after the Yetus build. |
💔 -1 overall
This message was automatically generated. |
Is the build system wrong? I don't think changed the code which cause the compile failure. |
/retest |
+1, I've merged the change to trunk. |
Thanks @xiaoyuyao for the code review and @elek for monitor the build result. |
…n NetworkTopology (apache#1194)
…n NetworkTopology (apache#1194)
https://issues.apache.org/jira/browse/HDDS-1879