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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
