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/04/30 07:59:25 UTC
[GitHub] [ozone] wzhallright opened a new pull request #2202: HDDS-5171. Make the report max limit configurable
wzhallright opened a new pull request #2202:
URL: https://github.com/apache/ozone/pull/2202
## What changes were proposed in this pull request?
Make the report max limit configurable.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5171
## How was this patch tested?
The original UT can cover this.
--
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.
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] adoroszlai commented on pull request #2202: HDDS-5171. Make the report max limit configurable
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2202:
URL: https://github.com/apache/ozone/pull/2202#issuecomment-900996900
/pending a use case of this configuration from user/administrator point of view
--
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] adoroszlai commented on a change in pull request #2202: HDDS-5171. Make the report max limit configurable
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2202:
URL: https://github.com/apache/ozone/pull/2202#discussion_r636278897
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HddsServerUtil.java
##########
@@ -325,6 +327,16 @@ public static int getScmRpcRetryCount(ConfigurationSource conf) {
OZONE_SCM_HEARTBEAT_RPC_RETRY_COUNT_DEFAULT);
}
+ /**
+ * Get the max limit when get all available reports.
+ * @param conf
+ * @return max limit
+ */
+ public static int getReportMaxLimit(ConfigurationSource conf) {
+ return conf.getInt(HDDS_DATANODE_REPORT_MAX_LIMIT,
+ HDDS_DATANODE_REPORT_MAX_LIMIT_DEFAULT);
+ }
+
Review comment:
Nit: the name `getReportMaxLimit` seems too generic when placed in the utility class. Can you please either move it to some datanode-specific class (e.g. `StateContext` itself), or rename it to be more specific?
--
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.
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] adoroszlai commented on pull request #2202: HDDS-5171. Make the report max limit configurable
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2202:
URL: https://github.com/apache/ozone/pull/2202#issuecomment-900996900
/pending a use case of this configuration from user/administrator point of view
--
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] adoroszlai commented on a change in pull request #2202: HDDS-5171. Make the report max limit configurable
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2202:
URL: https://github.com/apache/ozone/pull/2202#discussion_r639738573
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HddsServerUtil.java
##########
@@ -325,6 +327,16 @@ public static int getScmRpcRetryCount(ConfigurationSource conf) {
OZONE_SCM_HEARTBEAT_RPC_RETRY_COUNT_DEFAULT);
}
+ /**
+ * Get the max limit when get all available reports.
+ * @param conf
+ * @return max limit
+ */
+ public static int getReportMaxLimit(ConfigurationSource conf) {
+ return conf.getInt(HDDS_DATANODE_REPORT_MAX_LIMIT,
+ HDDS_DATANODE_REPORT_MAX_LIMIT_DEFAULT);
+ }
+
Review comment:
Thanks for the answers, and updating the patch.
> 1. In this code, the max reports size of getNonIncrementalReports() return is 3, if the maxLimit is Integer.MAX_VALUE, so maxLimit < 0 and maxLimit <= reports.size() is meaningless. I think the maxLimit need to become configurable.
The checks are not necessarily meaningless, may simply be defensive programming. Currently the only production caller passes `Integer.MAX_VALUE`. But the method is public, and new calls may be introduced where different limits might make sense. In this case they can be kept without making the limit used by `StateContext` in `getReports()` configurable.
Or they may be unnecessary and could be simply removed.
I was looking for a use case of this configuration from user/administrator point of view, not based on the code. If we don't know why it may be configured to different values (i.e. use cases), and how users should come up with those, then I think it may be better to avoid adding the config.
But that's just my 2 cents.
--
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.
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] wzhallright commented on a change in pull request #2202: HDDS-5171. Make the report max limit configurable
Posted by GitBox <gi...@apache.org>.
wzhallright commented on a change in pull request #2202:
URL: https://github.com/apache/ozone/pull/2202#discussion_r636724620
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HddsServerUtil.java
##########
@@ -325,6 +327,16 @@ public static int getScmRpcRetryCount(ConfigurationSource conf) {
OZONE_SCM_HEARTBEAT_RPC_RETRY_COUNT_DEFAULT);
}
+ /**
+ * Get the max limit when get all available reports.
+ * @param conf
+ * @return max limit
+ */
+ public static int getReportMaxLimit(ConfigurationSource conf) {
+ return conf.getInt(HDDS_DATANODE_REPORT_MAX_LIMIT,
+ HDDS_DATANODE_REPORT_MAX_LIMIT_DEFAULT);
+ }
+
Review comment:
Thanks @adoroszlai for review.
```
`public List<GeneratedMessage> getReports(InetSocketAddress endpoint,
int maxLimit) {
if (maxLimit < 0) {
throw new IllegalArgumentException("Illegal maxLimit value: " + maxLimit);
}
List<GeneratedMessage> reports = getNonIncrementalReports();
if (maxLimit <= reports.size()) {
return reports.subList(0, maxLimit);
} else {
reports.addAll(getIncrementalReports(endpoint,
maxLimit - reports.size()));
return reports;
}
}`
```
1) In this code, the max reports size of getNonIncrementalReports() return is 3, if the maxLimit is Integer.MAX_VALUE, so `maxLimit < 0` and `maxLimit <= reports.size()` is meaningless.
I think the maxLimit need to become configurable.
2) I will add it to ozone-default.xml instead of TestOzoneConfigurationFields.
3) Move getReportMaxLimit to StateContext.java.
--
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.
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] github-actions[bot] closed pull request #2202: HDDS-5171. Make the report max limit configurable
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #2202:
URL: https://github.com/apache/ozone/pull/2202
--
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] wzhallright commented on pull request #2202: HDDS-5171. Make the report max limit configurable
Posted by GitBox <gi...@apache.org>.
wzhallright commented on pull request #2202:
URL: https://github.com/apache/ozone/pull/2202#issuecomment-847609875
@adoroszlai would you help to take another look again?
--
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.
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] github-actions[bot] commented on pull request #2202: HDDS-5171. Make the report max limit configurable
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #2202:
URL: https://github.com/apache/ozone/pull/2202#issuecomment-915655047
Thank you very much for the patch. I am closing this PR __temporarily__ as there was no activity recently and it is waiting for response from its author.
It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.
It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.
If you need ANY help to finish this PR, please [contact the community](https://github.com/apache/hadoop-ozone#contact) on the mailing list or the slack channel."
--
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] wzhallright commented on a change in pull request #2202: HDDS-5171. Make the report max limit configurable
Posted by GitBox <gi...@apache.org>.
wzhallright commented on a change in pull request #2202:
URL: https://github.com/apache/ozone/pull/2202#discussion_r636724620
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HddsServerUtil.java
##########
@@ -325,6 +327,16 @@ public static int getScmRpcRetryCount(ConfigurationSource conf) {
OZONE_SCM_HEARTBEAT_RPC_RETRY_COUNT_DEFAULT);
}
+ /**
+ * Get the max limit when get all available reports.
+ * @param conf
+ * @return max limit
+ */
+ public static int getReportMaxLimit(ConfigurationSource conf) {
+ return conf.getInt(HDDS_DATANODE_REPORT_MAX_LIMIT,
+ HDDS_DATANODE_REPORT_MAX_LIMIT_DEFAULT);
+ }
+
Review comment:
Thanks @adoroszlai for review.
`public List<GeneratedMessage> getReports(InetSocketAddress endpoint,
int maxLimit) {
if (maxLimit < 0) {
throw new IllegalArgumentException("Illegal maxLimit value: " + maxLimit);
}
List<GeneratedMessage> reports = getNonIncrementalReports();
if (maxLimit <= reports.size()) {
return reports.subList(0, maxLimit);
} else {
reports.addAll(getIncrementalReports(endpoint,
maxLimit - reports.size()));
return reports;
}
}`
1) In this code, the max reports size of getNonIncrementalReports() return is 3, if the maxLimit is Integer.MAX_VALUE, so `maxLimit < 0` and `maxLimit <= reports.size()` is meaningless.
I think the maxLimit need to become configurable.
2) I will add it to ozone-default.xml instead of TestOzoneConfigurationFields.
3) Move getReportMaxLimit to StateContext.java.
--
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.
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] wzhallright commented on a change in pull request #2202: HDDS-5171. Make the report max limit configurable
Posted by GitBox <gi...@apache.org>.
wzhallright commented on a change in pull request #2202:
URL: https://github.com/apache/ozone/pull/2202#discussion_r636724620
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HddsServerUtil.java
##########
@@ -325,6 +327,16 @@ public static int getScmRpcRetryCount(ConfigurationSource conf) {
OZONE_SCM_HEARTBEAT_RPC_RETRY_COUNT_DEFAULT);
}
+ /**
+ * Get the max limit when get all available reports.
+ * @param conf
+ * @return max limit
+ */
+ public static int getReportMaxLimit(ConfigurationSource conf) {
+ return conf.getInt(HDDS_DATANODE_REPORT_MAX_LIMIT,
+ HDDS_DATANODE_REPORT_MAX_LIMIT_DEFAULT);
+ }
+
Review comment:
Thanks @adoroszlai for review.
`public List<GeneratedMessage> getReports(InetSocketAddress endpoint,
int maxLimit) {
if (maxLimit < 0) {
throw new IllegalArgumentException("Illegal maxLimit value: " + maxLimit);
}
List<GeneratedMessage> reports = getNonIncrementalReports();
if (maxLimit <= reports.size()) {
return reports.subList(0, maxLimit);
} else {
reports.addAll(getIncrementalReports(endpoint,
maxLimit - reports.size()));
return reports;
}
}`
1) In this code, the max reports size of getNonIncrementalReports() return is 3, if the maxLimit is Integer.MAX_VALUE, so `maxLimit < 0` and `maxLimit <= reports.size()` is meaningless.
I think the maxLimit need to become configurable.
2)I will add it to ozone-default.xml instead of TestOzoneConfigurationFields.
3)Move getReportMaxLimit to StateContext.java.
--
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.
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