You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/08/27 07:40:08 UTC
[GitHub] [ozone] ChenSammi opened a new pull request #2590: HDDS-5689. chooseRandom in NetworkTopology should exclude the affinity node too
ChenSammi opened a new pull request #2590:
URL: https://github.com/apache/ozone/pull/2590
https://issues.apache.org/jira/browse/HDDS-5689
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2590: HDDS-5689. chooseRandom in NetworkTopology should exclude the affinity node too
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2590:
URL: https://github.com/apache/ozone/pull/2590#discussion_r697392469
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/NetworkTopologyImpl.java
##########
@@ -514,6 +514,13 @@ private Node chooseNodeInternal(String scope, int leafIndex,
List<String> excludedScopes, Collection<Node> excludedNodes,
Node affinityNode, int ancestorGen) {
Preconditions.checkArgument(scope != null);
+ LOG.debug("Start choosing node[scope = {}, index = {}, excludedScopes = {},"
Review comment:
@sodonnel , {} will not be evaluated until debug is enabled. You can refer to this document,
http://www.slf4j.org/faq.html#logging_performance# What is the fastest way of (not) logging?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi merged pull request #2590: HDDS-5689. chooseRandom in NetworkTopology should exclude the affinity node too
Posted by GitBox <gi...@apache.org>.
ChenSammi merged pull request #2590:
URL: https://github.com/apache/ozone/pull/2590
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #2590: HDDS-5689. chooseRandom in NetworkTopology should exclude the affinity node too
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2590:
URL: https://github.com/apache/ozone/pull/2590#discussion_r697284341
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/NetworkTopologyImpl.java
##########
@@ -514,6 +514,13 @@ private Node chooseNodeInternal(String scope, int leafIndex,
List<String> excludedScopes, Collection<Node> excludedNodes,
Node affinityNode, int ancestorGen) {
Preconditions.checkArgument(scope != null);
+ LOG.debug("Start choosing node[scope = {}, index = {}, excludedScopes = {},"
Review comment:
This log message needs wrapped in a `if LOG.isDebugEnabled()`, as there is a lot of evaluation in the parameters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #2590: HDDS-5689. chooseRandom in NetworkTopology should exclude the affinity node too
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #2590:
URL: https://github.com/apache/ozone/pull/2590#issuecomment-908157962
Thanks @sodonnel for the code review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2590: HDDS-5689. chooseRandom in NetworkTopology should exclude the affinity node too
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2590:
URL: https://github.com/apache/ozone/pull/2590#discussion_r698126774
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/NetworkTopologyImpl.java
##########
@@ -514,6 +514,13 @@ private Node chooseNodeInternal(String scope, int leafIndex,
List<String> excludedScopes, Collection<Node> excludedNodes,
Node affinityNode, int ancestorGen) {
Preconditions.checkArgument(scope != null);
+ LOG.debug("Start choosing node[scope = {}, index = {}, excludedScopes = {},"
Review comment:
Thanks for the info. I have uploaded a new commit for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #2590: HDDS-5689. chooseRandom in NetworkTopology should exclude the affinity node too
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2590:
URL: https://github.com/apache/ozone/pull/2590#discussion_r697399215
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/NetworkTopologyImpl.java
##########
@@ -514,6 +514,13 @@ private Node chooseNodeInternal(String scope, int leafIndex,
List<String> excludedScopes, Collection<Node> excludedNodes,
Node affinityNode, int ancestorGen) {
Preconditions.checkArgument(scope != null);
+ LOG.debug("Start choosing node[scope = {}, index = {}, excludedScopes = {},"
Review comment:
That only holds true for concatenating the the values into the log string. If you pass complex values into the method, they are evaluated before the method is called and their result is passed into the method.
As the parameters are expressions which need evaluated, all that will work happen to create their values, but then the LOG.debug call will fail to use them unless debug is enabled. If you are passing simple variables string into the log method, the `if LOG.isdebugEnable()` is not needed (eg in the other case below in this same PR), but for this log, I think it is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #2590: HDDS-5689. chooseRandom in NetworkTopology should exclude the affinity node too
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #2590:
URL: https://github.com/apache/ozone/pull/2590#discussion_r698126774
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/NetworkTopologyImpl.java
##########
@@ -514,6 +514,13 @@ private Node chooseNodeInternal(String scope, int leafIndex,
List<String> excludedScopes, Collection<Node> excludedNodes,
Node affinityNode, int ancestorGen) {
Preconditions.checkArgument(scope != null);
+ LOG.debug("Start choosing node[scope = {}, index = {}, excludedScopes = {},"
Review comment:
Thanks for the info. I have updated a new commit for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org