FrankChen021 commented on PR #13034:
URL: https://github.com/apache/druid/pull/13034#issuecomment-1240548397
> > The changes look good. I have some concerns about thread-safety though.
The `Metrics` object or the map of `registeredMetrics` inside it is not
thread-safe. Every invocation of `emit` updates the map of registered which is
being read by `flush` on a different thread. So it's possible that the metric
map gets updated by an `emit` event while a `flush` event is still reading from
the map, thus sending a metric packet with inconsistent values.
> > I am not sure it would be a good idea to block `emit` while `flush` is
going on either.
> > @599166320 , @FrankChen021 , what do you think?
>
> >
>
> I think we should initialize registeredMetrics in the constructor to
ensure that it cannot be modified anywhere outside the constructor, like the
following code:
>
> ```
> public Metrics(String namespace, String path, boolean isAddHostAsLabel,
boolean isAddServiceAsLabel)
> {
> Map<String, DimensionsAndCollector> registeredMetricsTmp = new
HashMap<>();
> Map<String, Metric> metrics = readConfig(path);
> for (Map.Entry<String, Metric> entry : metrics.entrySet()) {
> //......
> if (collector != null) {
> registeredMetricsTmp.put(name, new
DimensionsAndCollector(dimensions, collector, metric.conversionFactor));
> }
> }
> this.registeredMetrics =
Collections.unmodifiableMap(registeredMetricsTmp);
> }
> ```
>
> What do you think?
Looks good to me.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
