[FLINK-7368][metrics] Make MetricStore ThreadSafe class #4840

pnowojski · 2017-10-16T15:00:58Z

Remove external synchronisation on MetricStore

What is the purpose of the change

This is a refactor that makes MetricStore thread safe. It achieves it by pulling in all modifying methods and by returning immutable copies in getters.

Verifying this change

This change is already covered by existing tests, such as MetricStoreTest or MetricFetcherTest

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)

Remove external synchronization on MetricStore

zentol · 2017-10-18T23:01:25Z

...-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java

+	 *
+	 * @param activeTaskManagers to retain.
+	 */
+	public synchronized void retainTaskManagers(List<String> activeTaskManagers) {


Isn't the method synchronization unnecessary now?

Strictly speaking right now it isn't. However without synchronising those methods, implementing them is much harder. For example I made one mistake where I implemented:

if (!jobs.containKey(jobID)) { return null; } return jobs.get(jobID).getTaskMetricStore(taskID)

Also without synchronising it's harder to understand correctness of code, that's handling three separate fields (whether this is a correct access order; whether the state of variable is/should be coherent, like if existence of key in one map, should imply existence a key on another, etc).

Long story short, synchronising is not an issue here, but helps with reasoning about this code.

but now we're paying the synchronization costs twice, once for the synchronized keyword and once again for every access to the map. If every method is synchronized the maps don't have to be concurrent hash maps.

performance here is not that big a deal, but code correctness is. With synchronized it is just easier to implement any changes here in a thread safe manner. Without it, any new developer coming here will have to understand much more assumptions about this code, like whether consistency matters here or nor? whether order of the operations/access to the fields is important or nor? etc...

even with synchronized we still need either concurrent hash maps, because we return and make them visible to the outside world by getters. So we either need concurrent hash maps or return copies of them.

But I'm not that strongly opinionated about this one. If after this comment you still would like to drop those synchronized, I will do it.

zentol · 2017-10-24T13:05:09Z

...-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java

+	 *
+	 * @param activeTaskManagers to retain.
+	 */
+	public synchronized void retainTaskManagers(List<String> activeTaskManagers) {


but now we're paying the synchronization costs twice, once for the synchronized keyword and once again for every access to the map. If every method is synchronized the maps don't have to be concurrent hash maps.

zentol · 2017-10-24T13:10:27Z

...-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java


 		public void addGarbageCollectorName(String name) {
 			garbageCollectorNames.add(name);
 		}
+
+		public static TaskManagerMetricStore unmodifiable(TaskManagerMetricStore source) {


I'm wondering whether we really need this unmodifiable business. Yes, it's technically a good idea, but the access to the MetricStore is limited and fully under our control; so we know that we never try to modify the map.

It's a matter of clean design and maintaining it in the future. You only know that we do not modify this map at the moment. Are you sure that every committer reviewing changes in this parts of the code will always spot any such modifications without unmodifiable assertion? On the other hand removing unmodifiable in a PR is big yellow warning light that's something is going on.

Following this logic, why don't we make all methods/fields public, since we know that we will never try to access supposed to be private fields?

In which case we should rather modify the store to not allow writes in the first place, instead of opting for unmodifiable collections that are pretty much a hack. "here, have an object that fails for 50% of the defined methods"; that's hardly good design is it.

Till suggested dedicated read methods that return metrics, something like List<Tuple<String, String>> getMetrics(List<String> metricNames) instead of exposing the metric maps.

This would make the interface cleaner and would allow us to simplify the synchronization.

Would you rather have a runtime error about illegal/unintended write access or none at all? Java sucks in this regard (missing const-correctness) :(

Exposing List<...> have the same issue.

If the returned list is a fresh copy independent of the map it can be modifiable; the user can do whatever he wants with it. There are no side-effects on the MetricStore which as i understand is the reasoning behind making things unmodifiable.

Then you pay costs of copying all of the objects, which is much higher compared to the synchronisation. But yes, you can do that. In that case you don't need concurrent hash maps.

If you intend to go in other direction with this and leave the current code in the master as it is, please log the work in Jira for this after the release (I will close release blocker issue for which I started this PR). However I have an impression that any follow up work would be easier on top of this change and in the mean time this is still way better then current code on master branch with this external synchronisation.

zentol · 2017-10-25T11:14:06Z

flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/util/MutableIOMetrics.java

@@ -79,7 +79,7 @@ public void addIOMetrics(AccessExecution attempt, @Nullable MetricFetcher fetche
 				fetcher.update();
 				MetricStore metricStore = fetcher.getMetricStore();
 				synchronized (metricStore) {


leftover synchronization

Don't know why I have missed it :(

zentol · 2017-10-25T11:14:33Z

...-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java

+	 *
+	 * @param activeJobs to retain.
+	 */
+	public synchronized void retainJobs(List<String> activeJobs) {


can be package private

zentol · 2017-10-25T11:14:38Z

...-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java

+	 *
+	 * @param metricDumps to add.
+	 */
+	public synchronized void addAll(List<MetricDump> metricDumps) {


package private

zentol · 2017-10-25T11:16:23Z

...-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java

-	public void add(MetricDump metric) {
+
+	/**
+	 * Returns the {@link ComponentMetricStore}.


javadoc is missing any mention of jobmanager

zentol · 2017-10-25T11:16:37Z

...-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java

+	/**
+	 * Returns the {@link ComponentMetricStore}.
+	 *
+	 * @return JobManagerMetricStore


outdated class reference

zentol · 2017-10-25T11:17:51Z

...-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java

-		JobMetricStore job = getJobMetricStore(jobID);
-		if (job == null) {
-			return null;
+		public ComponentMetricStore() {


all constructors can be private

zentol · 2017-10-25T11:18:30Z

...-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java

-	 * Sub-structure containing metrics of the JobManager.
-	 */
-	public static class JobManagerMetricStore extends ComponentMetricStore {
+		public static ComponentMetricStore unmodifiable(ComponentMetricStore source) {


can be private

zentol · 2017-10-25T11:19:08Z

...time/src/test/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStoreTest.java

-		assertEquals(0, store.jobManager.metrics.size());
-		assertEquals(0, store.taskManagers.size());
-		assertEquals(0, store.jobs.size());
+		assertTrue(store.getTaskManagers().isEmpty());


i prefer the previous version as it prints the actual value.

zentol

merging.

Remove external synchronization on MetricStore This closes apache#4840.

pnowojski · 2017-10-25T13:05:24Z

Thanks!

Remove external synchronization on MetricStore This closes apache#4840.

pnowojski force-pushed the flink7368 branch 3 times, most recently from 7442005 to 27eb459 Compare October 18, 2017 11:15

[FLINK-7368][metrics] Make MetricStore ThreadSafe class

b190774

Remove external synchronization on MetricStore

pnowojski force-pushed the flink7368 branch from 27eb459 to b190774 Compare October 18, 2017 16:42

zentol reviewed Oct 18, 2017

View reviewed changes

zentol reviewed Oct 24, 2017

View reviewed changes

zentol reviewed Oct 25, 2017

View reviewed changes

fixup! [FLINK-7368][metrics] Make MetricStore ThreadSafe class

b6be2ce

zentol approved these changes Oct 25, 2017

View reviewed changes

zentol pushed a commit to zentol/flink that referenced this pull request Oct 25, 2017

[FLINK-7368][metrics] Make MetricStore ThreadSafe class

d504ebd

Remove external synchronization on MetricStore This closes apache#4840.

pnowojski closed this Oct 25, 2017

pnowojski reopened this Oct 25, 2017

zentol pushed a commit to zentol/flink that referenced this pull request Oct 26, 2017

[FLINK-7368][metrics] Make MetricStore ThreadSafe class

c90489f

Remove external synchronization on MetricStore This closes apache#4840.

asfgit closed this in f622de3 Oct 26, 2017

rmetzger added the component=Runtime/Metrics label Mar 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-7368][metrics] Make MetricStore ThreadSafe class #4840

[FLINK-7368][metrics] Make MetricStore ThreadSafe class #4840

pnowojski commented Oct 16, 2017

zentol Oct 18, 2017

pnowojski Oct 19, 2017

zentol Oct 24, 2017

pnowojski Oct 24, 2017 •

edited

zentol Oct 24, 2017

zentol Oct 24, 2017

pnowojski Oct 24, 2017

zentol Oct 24, 2017 •

edited

pnowojski Oct 24, 2017

zentol Oct 24, 2017 •

edited

pnowojski Oct 25, 2017

zentol Oct 25, 2017

pnowojski Oct 25, 2017

zentol Oct 25, 2017

zentol Oct 25, 2017

zentol Oct 25, 2017

zentol Oct 25, 2017

zentol Oct 25, 2017

zentol Oct 25, 2017

zentol Oct 25, 2017 •

edited

zentol left a comment

pnowojski commented Oct 25, 2017

[FLINK-7368][metrics] Make MetricStore ThreadSafe class #4840

[FLINK-7368][metrics] Make MetricStore ThreadSafe class #4840

Conversation

pnowojski commented Oct 16, 2017

What is the purpose of the change

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnowojski Oct 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zentol Oct 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zentol Oct 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zentol Oct 25, 2017 • edited

Choose a reason for hiding this comment

zentol left a comment

Choose a reason for hiding this comment

pnowojski commented Oct 25, 2017

pnowojski Oct 24, 2017 •

edited

zentol Oct 24, 2017 •

edited

zentol Oct 24, 2017 •

edited

zentol Oct 25, 2017 •

edited