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