Adding support for Dhalion to fetch metrics from multiple metrics pro…#44
Adding support for Dhalion to fetch metrics from multiple metrics pro…#44abmodi wants to merge 1 commit intomicrosoft:masterfrom
Conversation
ashvina
left a comment
ashvina
left a comment
There was a problem hiding this comment.
In Dhalion, we have been using 120 column size lines instead of the hadoop style of 80 column lines. Could you please reformat accordingly.
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| public class CompositeMetricsProvider implements MetricsProvider { |
There was a problem hiding this comment.
This class needs some a javadoc to help users know the behavior of this implementation
|
|
||
| @VisibleForTesting | ||
| protected void instantiateMetricsProviders() throws ClassNotFoundException { | ||
| Injector injector = Guice.createInjector(new AbstractModule() { |
There was a problem hiding this comment.
Creating an injector here does not seem correct. This injector can only be used for instantiating metrics providers that need sysconfig. This may not be sufficient. If at all there needs to be a way to pass the injector in the constructor and use it.
| } | ||
| } | ||
|
|
||
| protected void addMetricsProvider(MetricsProvider provider) { |
There was a problem hiding this comment.
Is this protected for testing?
| metricsProvider.initialize(); | ||
| Set<String> metricTypes = metricsProvider.getMetricTypes(); | ||
| if (metricTypes != null) { | ||
| for (String metricType : metricsProvider.getMetricTypes()) { |
There was a problem hiding this comment.
Reuse the metric type set above?
| for (String metricType : metricsProvider.getMetricTypes()) { | |
| for (String metricType : metricTypes) { |
| Set<String> metricTypes = metricsProvider.getMetricTypes(); | ||
| if (metricTypes != null) { | ||
| for (String metricType : metricsProvider.getMetricTypes()) { | ||
| metricsProviderMap.put(metricType, metricsProvider); |
There was a problem hiding this comment.
Should an exception be thrown if more than one metrics provider supports a metric type?
2 participants
…viders.