You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/09/20 20:25:04 UTC
[GitHub] [helix] pkuwm opened a new pull request #1383: Add Metrics for External Custom Health Check Requests
pkuwm opened a new pull request #1383:
URL: https://github.com/apache/helix/pull/1383
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
Resolves #1382
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
This is a draft because we need to discuss and trade-off design.
Custom rest client interacts with participant side to query for its health checks. Currently there is no metrics for the query requests. We'd like to have metrics for the requests.
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- [ ] The following is the result of the "mvn test" command on the appropriate module:
(Before CI test pass, please copy & paste the result of "mvn test")
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r496318526
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -166,12 +187,18 @@ public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor, CustomRestClie
private Map<String, Boolean> getHealthStatusFromRest(String instance, List<String> partitions,
RESTConfig restConfig, Map<String, String> customPayLoads) {
- try {
+ MetricRegistry metrics = SharedMetricRegistries.getOrCreate(_namespace);
+ Counter requestsTotal = metrics.counter(CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL);
+ Counter errorRequestsTotal = metrics.counter(CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_ERROR_TOTAL);
+ try (final Timer.Context timer = metrics.timer(CUSTOM_PARTITION_CHECK_HTTP_REQUEST_DURATION)
Review comment:
It won't make the duration statistic too bad, compared with network IO. CPU is really fast to process the exception. Actually we still need to use a `try... finally` block - same as this `try.. with..resource`.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -54,15 +60,30 @@
public static final String IS_HEALTHY_KEY = "IS_HEALTHY";
public static final String EXPIRY_KEY = "EXPIRE";
+ // Metric names for custom partition check
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_requests_total");
Review comment:
In the object name, there is a `type` property for us to know that: `type=counters` or `type=gauges`. Then we'll use the `type` property.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -58,9 +63,18 @@
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
private static final ExecutorService POOL = Executors.newCachedThreadPool();
+ // Metric names for custom instance check
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_requests_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_requests_error_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_request_duration");
+
private final HelixDataAccessorWrapper _dataAccessor;
Review comment:
No :) You got it. The first PR was a draft. A constructor with namespace is passed in, I didn't push the change yet when you saw this. Updated.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -54,15 +60,30 @@
public static final String IS_HEALTHY_KEY = "IS_HEALTHY";
public static final String EXPIRY_KEY = "EXPIRE";
+ // Metric names for custom partition check
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_requests_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_ERROR_TOTAL = MetricRegistry
+ .name(InstanceService.class, "custom_partition_check_http_requests_error_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_request_duration");
+
private final Map<PropertyKey, HelixProperty> _propertyCache = new HashMap<>();
private final Map<PropertyKey, List<String>> _batchNameCache = new HashMap<>();
+ private String _namespace;
protected CustomRestClient _restClient;
public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
Review comment:
I was thinking other callers may use it. Within helix, only a mock class calls it. So I just changed it and deprecate this, assuming this constructor is supposed to be used within helix rest.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -231,14 +252,22 @@ private StoppableCheck performHelixOwnInstanceCheck(String clusterId, String ins
private StoppableCheck performCustomInstanceCheck(String clusterId, String instanceName,
String baseUrl, Map<String, String> customPayLoads) {
LOG.info("Perform instance level client side health checks for {}/{}", clusterId, instanceName);
- try {
- return new StoppableCheck(
- _customRestClient.getInstanceStoppableCheck(baseUrl, customPayLoads),
+ MetricRegistry metrics = SharedMetricRegistries.getOrCreate(_namespace);
+ Counter requestsTotal = metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL);
+ Counter errorRequestsTotal = metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL);
+
+ try (final Timer.Context timer = metrics.timer(CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION)
Review comment:
Java 8 `try..with..resource` needs this variable, though we won't use it in the block :). Maybe a newer java supports it.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -58,9 +63,18 @@
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
private static final ExecutorService POOL = Executors.newCachedThreadPool();
+ // Metric names for custom instance check
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL =
Review comment:
This naming is based on the method/function. It is a generic interface api `getInstanceStoppableCheck`, so no matter it is a health check or else, as long as it calls this api, we record it. What do you think?
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r496326240
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -54,15 +60,30 @@
public static final String IS_HEALTHY_KEY = "IS_HEALTHY";
public static final String EXPIRY_KEY = "EXPIRE";
+ // Metric names for custom partition check
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_requests_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_ERROR_TOTAL = MetricRegistry
+ .name(InstanceService.class, "custom_partition_check_http_requests_error_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_request_duration");
+
private final Map<PropertyKey, HelixProperty> _propertyCache = new HashMap<>();
private final Map<PropertyKey, List<String>> _batchNameCache = new HashMap<>();
+ private String _namespace;
Review comment:
In helix rest logic, I think we need to pass the namespace to this wrapper so partition check metrics could be namespaced, otherwise, we won't be able to record namespaced metrics for this partition check.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r496334891
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -231,14 +252,22 @@ private StoppableCheck performHelixOwnInstanceCheck(String clusterId, String ins
private StoppableCheck performCustomInstanceCheck(String clusterId, String instanceName,
String baseUrl, Map<String, String> customPayLoads) {
LOG.info("Perform instance level client side health checks for {}/{}", clusterId, instanceName);
- try {
- return new StoppableCheck(
- _customRestClient.getInstanceStoppableCheck(baseUrl, customPayLoads),
+ MetricRegistry metrics = SharedMetricRegistries.getOrCreate(_namespace);
+ Counter requestsTotal = metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL);
+ Counter errorRequestsTotal = metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL);
Review comment:
Resolved.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r496437740
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -58,9 +63,18 @@
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
private static final ExecutorService POOL = Executors.newCachedThreadPool();
+ // Metric names for custom instance check
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_requests_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_requests_error_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_request_duration");
+
private final HelixDataAccessorWrapper _dataAccessor;
Review comment:
As commented above, passing the namespace twice in one line of code is not a good design. If you are convinced that the wrapper object should be limited so serve only one service impl, then let's move it inside.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm merged pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
pkuwm merged pull request #1383:
URL: https://github.com/apache/helix/pull/1383
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r496424250
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -231,14 +252,22 @@ private StoppableCheck performHelixOwnInstanceCheck(String clusterId, String ins
private StoppableCheck performCustomInstanceCheck(String clusterId, String instanceName,
String baseUrl, Map<String, String> customPayLoads) {
LOG.info("Perform instance level client side health checks for {}/{}", clusterId, instanceName);
- try {
- return new StoppableCheck(
- _customRestClient.getInstanceStoppableCheck(baseUrl, customPayLoads),
+ MetricRegistry metrics = SharedMetricRegistries.getOrCreate(_namespace);
+ Counter requestsTotal = metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL);
+ Counter errorRequestsTotal = metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL);
+
+ try (final Timer.Context timer = metrics.timer(CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION)
Review comment:
I see. It needs a local variable to do the trick. It looks strange but as long as it works, I'm good.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r496325656
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/HelixRestServer.java
##########
@@ -222,7 +222,6 @@ public void shutdown() {
}
}
_jmxReporterList.forEach(JmxReporter::stop);
- _jmxReporterList.clear();
Review comment:
Somehow happened when rebasing/merging code from another local branch. Not intended.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r492454847
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -231,14 +252,22 @@ private StoppableCheck performHelixOwnInstanceCheck(String clusterId, String ins
private StoppableCheck performCustomInstanceCheck(String clusterId, String instanceName,
String baseUrl, Map<String, String> customPayLoads) {
LOG.info("Perform instance level client side health checks for {}/{}", clusterId, instanceName);
- try {
- return new StoppableCheck(
- _customRestClient.getInstanceStoppableCheck(baseUrl, customPayLoads),
+ MetricRegistry metrics = SharedMetricRegistries.getOrCreate(_namespace);
+ Counter requestsTotal = metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL);
+ Counter errorRequestsTotal = metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL);
Review comment:
nit: variable name to be consistent with metric name to avoid misuse.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -58,9 +63,18 @@
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
private static final ExecutorService POOL = Executors.newCachedThreadPool();
+ // Metric names for custom instance check
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL =
Review comment:
More specifically, this is "health" check. Maybe you can make it more accurate in case you have other types of instance check metrics later and need to distinguish between them.
Same for partition health check.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -54,15 +60,30 @@
public static final String IS_HEALTHY_KEY = "IS_HEALTHY";
public static final String EXPIRY_KEY = "EXPIRE";
+ // Metric names for custom partition check
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_requests_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_ERROR_TOTAL = MetricRegistry
+ .name(InstanceService.class, "custom_partition_check_http_requests_error_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_request_duration");
+
private final Map<PropertyKey, HelixProperty> _propertyCache = new HashMap<>();
private final Map<PropertyKey, List<String>> _batchNameCache = new HashMap<>();
+ private String _namespace;
protected CustomRestClient _restClient;
public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
Review comment:
Shall we mark this as deprecated?
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -166,12 +187,18 @@ public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor, CustomRestClie
private Map<String, Boolean> getHealthStatusFromRest(String instance, List<String> partitions,
RESTConfig restConfig, Map<String, String> customPayLoads) {
- try {
+ MetricRegistry metrics = SharedMetricRegistries.getOrCreate(_namespace);
+ Counter requestsTotal = metrics.counter(CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL);
+ Counter errorRequestsTotal = metrics.counter(CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_ERROR_TOTAL);
+ try (final Timer.Context timer = metrics.timer(CUSTOM_PARTITION_CHECK_HTTP_REQUEST_DURATION)
Review comment:
In case of an exception, the time will be also counted in the request duration, right? Is this what in the design? I think this will make the duration statistic less accurate.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r495186197
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -54,15 +60,30 @@
public static final String IS_HEALTHY_KEY = "IS_HEALTHY";
public static final String EXPIRY_KEY = "EXPIRE";
+ // Metric names for custom partition check
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_requests_total");
Review comment:
How do we determine the metric type (gauge or counter) later when we want to expose the MBean attribute to the monitoring system?
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -231,14 +252,22 @@ private StoppableCheck performHelixOwnInstanceCheck(String clusterId, String ins
private StoppableCheck performCustomInstanceCheck(String clusterId, String instanceName,
String baseUrl, Map<String, String> customPayLoads) {
LOG.info("Perform instance level client side health checks for {}/{}", clusterId, instanceName);
- try {
- return new StoppableCheck(
- _customRestClient.getInstanceStoppableCheck(baseUrl, customPayLoads),
+ MetricRegistry metrics = SharedMetricRegistries.getOrCreate(_namespace);
+ Counter requestsTotal = metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL);
+ Counter errorRequestsTotal = metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL);
+
+ try (final Timer.Context timer = metrics.timer(CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION)
Review comment:
metrics.timer(CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION).time() is enough? You are not using the timer object in the try code block anyway, right?
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -166,12 +187,18 @@ public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor, CustomRestClie
private Map<String, Boolean> getHealthStatusFromRest(String instance, List<String> partitions,
RESTConfig restConfig, Map<String, String> customPayLoads) {
- try {
+ MetricRegistry metrics = SharedMetricRegistries.getOrCreate(_namespace);
+ Counter requestsTotal = metrics.counter(CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL);
+ Counter errorRequestsTotal = metrics.counter(CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_ERROR_TOTAL);
Review comment:
Please check my comments for the instance accessor, the same here. Please lazily get and simplify the code whenever possible.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -231,14 +252,22 @@ private StoppableCheck performHelixOwnInstanceCheck(String clusterId, String ins
private StoppableCheck performCustomInstanceCheck(String clusterId, String instanceName,
String baseUrl, Map<String, String> customPayLoads) {
LOG.info("Perform instance level client side health checks for {}/{}", clusterId, instanceName);
- try {
- return new StoppableCheck(
- _customRestClient.getInstanceStoppableCheck(baseUrl, customPayLoads),
+ MetricRegistry metrics = SharedMetricRegistries.getOrCreate(_namespace);
+ Counter requestsTotal = metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL);
+ Counter errorRequestsTotal = metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL);
Review comment:
It is lazily initialized, let's don't call it without a real error.
I would prefer just coding like the following, so you don't need the extra local fields def.
metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL).inc();
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/HelixRestServer.java
##########
@@ -222,7 +222,6 @@ public void shutdown() {
}
}
_jmxReporterList.forEach(JmxReporter::stop);
- _jmxReporterList.clear();
Review comment:
Why not clear?
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -54,15 +60,30 @@
public static final String IS_HEALTHY_KEY = "IS_HEALTHY";
public static final String EXPIRY_KEY = "EXPIRE";
+ // Metric names for custom partition check
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_requests_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_ERROR_TOTAL = MetricRegistry
+ .name(InstanceService.class, "custom_partition_check_http_requests_error_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_request_duration");
+
private final Map<PropertyKey, HelixProperty> _propertyCache = new HashMap<>();
private final Map<PropertyKey, List<String>> _batchNameCache = new HashMap<>();
+ private String _namespace;
Review comment:
_namespace is not a must logic for the class, so I suggest adding a set method for it instead of adding a constructor.
If the namespace is not set, then just skip the metrics logic.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -58,9 +63,18 @@
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
private static final ExecutorService POOL = Executors.newCachedThreadPool();
+ // Metric names for custom instance check
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_requests_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_requests_error_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_request_duration");
+
private final HelixDataAccessorWrapper _dataAccessor;
Review comment:
You can get namespace from dataAccessor? Please try to ensure there is only one source of truth.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -58,9 +63,18 @@
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
private static final ExecutorService POOL = Executors.newCachedThreadPool();
+ // Metric names for custom instance check
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_requests_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_requests_error_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_request_duration");
+
private final HelixDataAccessorWrapper _dataAccessor;
Review comment:
Or do we share the same accessor across multiple service implementation? If that's the case, we need to reconsider the design.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r496437136
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -54,15 +60,30 @@
public static final String IS_HEALTHY_KEY = "IS_HEALTHY";
public static final String EXPIRY_KEY = "EXPIRE";
+ // Metric names for custom partition check
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_requests_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_ERROR_TOTAL = MetricRegistry
+ .name(InstanceService.class, "custom_partition_check_http_requests_error_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_request_duration");
+
private final Map<PropertyKey, HelixProperty> _propertyCache = new HashMap<>();
private final Map<PropertyKey, List<String>> _batchNameCache = new HashMap<>();
+ private String _namespace;
Review comment:
High-levelly, I think we shall try our best to ensure that the metrics logic does not impact business logic. But the current implementation has 2 impacts at least.
1. More constructors, harder to use.
2. We enforce the business logic to have separate accessor wrapper for different namespaces. This does not make much sense if you consider the resource usage, object management, etc.
On the other hand, if we do not let the data accessor wrapper record the namespace, then the metrics can be recorded in the callers. This is a trade-off.
With the current implementation, I actually agree with put it inside the wrapper class. But we shall try to pass the namespace in a more direct way. For example, since we are enforcing the wrapper object serve for one namespace only, we can construct the wrapper object inside the InstanceServiceImpl. In this way, you are passing the namespace only once. And there is no way for the other devs to introduce inconsistent namespace 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r496420413
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -54,15 +60,30 @@
public static final String IS_HEALTHY_KEY = "IS_HEALTHY";
public static final String EXPIRY_KEY = "EXPIRE";
+ // Metric names for custom partition check
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_requests_total");
Review comment:
This is a good design actually. But it conflicts with our current assumption that the metric name contains GUAGE or COUNTER keyword. Please suggest how you think we shall let them work together? Or shall we modify our current metrics to include this type field too?
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r496438403
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -58,29 +62,44 @@
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
private static final ExecutorService POOL = Executors.newCachedThreadPool();
+ // Metric names for custom instance check
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_requests_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_requests_error_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_request_duration");
+
private final HelixDataAccessorWrapper _dataAccessor;
private final ConfigAccessor _configAccessor;
private final CustomRestClient _customRestClient;
+ private String _namespace;
private boolean _skipZKRead;
+ @Deprecated
public InstanceServiceImpl(HelixDataAccessorWrapper dataAccessor, ConfigAccessor configAccessor) {
- _dataAccessor = dataAccessor;
- _configAccessor = configAccessor;
- _customRestClient = CustomRestClientFactory.get();
+ this(dataAccessor, configAccessor, false);
}
- public InstanceServiceImpl(HelixDataAccessorWrapper dataAccessor, ConfigAccessor configAccessor, boolean skipZKRead) {
- this(dataAccessor,configAccessor);
- this._skipZKRead = skipZKRead;
+ @Deprecated
+ public InstanceServiceImpl(HelixDataAccessorWrapper dataAccessor, ConfigAccessor configAccessor,
+ boolean skipZKRead) {
+ this(dataAccessor, configAccessor, skipZKRead, HelixRestNamespace.DEFAULT_NAMESPACE_NAME);
+ }
Review comment:
I might be wrong, but helix-rest is not a module that we expect anyone to extend, so we shall be able to remove unnecessary methods directly.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r496592849
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -54,15 +60,30 @@
public static final String IS_HEALTHY_KEY = "IS_HEALTHY";
public static final String EXPIRY_KEY = "EXPIRE";
+ // Metric names for custom partition check
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_requests_total");
Review comment:
Helix-core is using keyword `Counter`/`Gauge` to represent the metric type. Either works. I'd like more inserting the type into object name, if we expect users to parse object names in mbean server. And it gives more flexibility/space to name a metric without Counter/Gauge. It can also be "time", "latency", "total", "requests_queued", etc..
More easily and concisely, I would suggest we can have a centralized metrics exposed to users so users can use the metrics class to get helix metrics.
```
HelixRestMetrics { // or HelixCoreMetrics
public final Counter custom_call_request;
public final Gauge request_latency;
}
```
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r492454847
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -231,14 +252,22 @@ private StoppableCheck performHelixOwnInstanceCheck(String clusterId, String ins
private StoppableCheck performCustomInstanceCheck(String clusterId, String instanceName,
String baseUrl, Map<String, String> customPayLoads) {
LOG.info("Perform instance level client side health checks for {}/{}", clusterId, instanceName);
- try {
- return new StoppableCheck(
- _customRestClient.getInstanceStoppableCheck(baseUrl, customPayLoads),
+ MetricRegistry metrics = SharedMetricRegistries.getOrCreate(_namespace);
+ Counter requestsTotal = metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL);
+ Counter errorRequestsTotal = metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL);
Review comment:
nit: variable name to be consistent with metric name to avoid misuse.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -58,9 +63,18 @@
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
private static final ExecutorService POOL = Executors.newCachedThreadPool();
+ // Metric names for custom instance check
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL =
Review comment:
More specifically, this is "health" check. Maybe you can make it more accurate in case you have other types of instance check metrics later and need to distinguish between them.
Same for partition health check.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -54,15 +60,30 @@
public static final String IS_HEALTHY_KEY = "IS_HEALTHY";
public static final String EXPIRY_KEY = "EXPIRE";
+ // Metric names for custom partition check
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_requests_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_ERROR_TOTAL = MetricRegistry
+ .name(InstanceService.class, "custom_partition_check_http_requests_error_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_request_duration");
+
private final Map<PropertyKey, HelixProperty> _propertyCache = new HashMap<>();
private final Map<PropertyKey, List<String>> _batchNameCache = new HashMap<>();
+ private String _namespace;
protected CustomRestClient _restClient;
public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
Review comment:
Shall we mark this as deprecated?
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -166,12 +187,18 @@ public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor, CustomRestClie
private Map<String, Boolean> getHealthStatusFromRest(String instance, List<String> partitions,
RESTConfig restConfig, Map<String, String> customPayLoads) {
- try {
+ MetricRegistry metrics = SharedMetricRegistries.getOrCreate(_namespace);
+ Counter requestsTotal = metrics.counter(CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL);
+ Counter errorRequestsTotal = metrics.counter(CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_ERROR_TOTAL);
+ try (final Timer.Context timer = metrics.timer(CUSTOM_PARTITION_CHECK_HTTP_REQUEST_DURATION)
Review comment:
In case of an exception, the time will be also counted in the request duration, right? Is this what in the design? I think this will make the duration statistic less accurate.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r497070180
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -58,9 +63,18 @@
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
private static final ExecutorService POOL = Executors.newCachedThreadPool();
+ // Metric names for custom instance check
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_requests_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_requests_error_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_request_duration");
+
private final HelixDataAccessorWrapper _dataAccessor;
Review comment:
Moved. Thx.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -58,29 +62,44 @@
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
private static final ExecutorService POOL = Executors.newCachedThreadPool();
+ // Metric names for custom instance check
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_requests_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_requests_error_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class, "custom_instance_check_http_request_duration");
+
private final HelixDataAccessorWrapper _dataAccessor;
private final ConfigAccessor _configAccessor;
private final CustomRestClient _customRestClient;
+ private String _namespace;
private boolean _skipZKRead;
+ @Deprecated
public InstanceServiceImpl(HelixDataAccessorWrapper dataAccessor, ConfigAccessor configAccessor) {
- _dataAccessor = dataAccessor;
- _configAccessor = configAccessor;
- _customRestClient = CustomRestClientFactory.get();
+ this(dataAccessor, configAccessor, false);
}
- public InstanceServiceImpl(HelixDataAccessorWrapper dataAccessor, ConfigAccessor configAccessor, boolean skipZKRead) {
- this(dataAccessor,configAccessor);
- this._skipZKRead = skipZKRead;
+ @Deprecated
+ public InstanceServiceImpl(HelixDataAccessorWrapper dataAccessor, ConfigAccessor configAccessor,
+ boolean skipZKRead) {
+ this(dataAccessor, configAccessor, skipZKRead, HelixRestNamespace.DEFAULT_NAMESPACE_NAME);
+ }
Review comment:
Since helix is open source, I prefer to follow a standard process: deprecate first and then remove it.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1383:
URL: https://github.com/apache/helix/pull/1383#issuecomment-701561320
Thanks, all, for the review!
This PR is ready to be merged, approved by @kaisun2000 @alirezazamani
```
Custom rest client interacts with participant side to query for its health checks. Currently there is no metrics for the query requests. We'd like to have metrics for the external http requests.
This commit adds http request latency, requests total and error http request total metrics for the custom rest client calls.
```
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1383: Add Metrics for External Custom Health Check Requests
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r497072991
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -54,15 +60,30 @@
public static final String IS_HEALTHY_KEY = "IS_HEALTHY";
public static final String EXPIRY_KEY = "EXPIRE";
+ // Metric names for custom partition check
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_requests_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_ERROR_TOTAL = MetricRegistry
+ .name(InstanceService.class, "custom_partition_check_http_requests_error_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class, "custom_partition_check_http_request_duration");
+
private final Map<PropertyKey, HelixProperty> _propertyCache = new HashMap<>();
private final Map<PropertyKey, List<String>> _batchNameCache = new HashMap<>();
+ private String _namespace;
Review comment:
This is a good suggestion. Previously I was not intended to change the constructor of `InstanceServiceImpl`. As you mentioned, helix rest is expected to be internal, so I think it is fine to change the signature. I have constructed the wrapper object inside the InstanceServiceImpl.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org