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