Add a cap to metrics communicator in TMasterSink and MetricsCacheSink #3355
Conversation
| while (!publishMetricsCommunicator.isEmpty()) { | ||
| TopologyMaster.PublishMetrics publishMetrics = publishMetricsCommunicator.poll(); | ||
| TopologyMaster.PublishMetrics publishMetrics; | ||
| synchronized (publishMetricsCommunicator) { |
There was a problem hiding this comment.
I see that previously we didn't use synchronized during operations on publishMetricsCommunicator.
What have changed? Do we now work with publishMetricsCommunicator in different threads?
There was a problem hiding this comment.
Yeah. Before there were only 1 producer (metrics manager) and 1 consumer (sink client) for this communicator from two different threads.
In the change, the producer can remove entries from head too so it is competing with the consumer thread. A simpler way is to avoid adding new entries into the queue, then we don't need to synchronize the producer and the consumer. However, I feel that the recent data could be more important than the old data.
| TopologyMaster.PublishMetrics publishMetrics; | ||
| synchronized (publishMetricsCommunicator) { | ||
| publishMetrics = publishMetricsCommunicator.poll(); | ||
| } |
There was a problem hiding this comment.
hmm, if we change the while loop condition, maybe we could reduce the code duplicate here?
There was a problem hiding this comment.
like while (synchronized (publishMetricsCommunicator) { publishMetrics = .... } ) {}?
As a non-java programmer, I don't know if it is ok. :D
But this is a valid point. Let me see if I can improve it.
|
👍 |
…apache#3355) * Add a cap to metrics communicator in TMasterSink and MetricsCacheSink * refactor while loop
…#3355) * Add a cap to metrics communicator in TMasterSink and MetricsCacheSink * refactor while loop
Without the cap, when MetricsCacheSink is enabled but MetricsCache is not running, the communication will keep growing.
The same thing could happen with TMaster but it is less likely to have the problem.