flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From pnowojski <...@git.apache.org>
Subject [GitHub] flink pull request #4840: [FLINK-7368][metrics] Make MetricStore ThreadSafe ...
Date Thu, 19 Oct 2017 06:50:28 GMT
Github user pnowojski commented on a diff in the pull request:

    https://github.com/apache/flink/pull/4840#discussion_r145611565
  
    --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/metrics/MetricStore.java
---
    @@ -38,29 +43,136 @@
     import static org.apache.flink.runtime.metrics.dump.QueryScopeInfo.INFO_CATEGORY_OPERATOR;
     import static org.apache.flink.runtime.metrics.dump.QueryScopeInfo.INFO_CATEGORY_TASK;
     import static org.apache.flink.runtime.metrics.dump.QueryScopeInfo.INFO_CATEGORY_TM;
    +import static org.apache.flink.util.Preconditions.checkNotNull;
     
     /**
      * Nested data-structure to store metrics.
    - *
    - * <p>This structure is not thread-safe.
      */
    +@ThreadSafe
     public class MetricStore {
     	private static final Logger LOG = LoggerFactory.getLogger(MetricStore.class);
     
    -	final JobManagerMetricStore jobManager = new JobManagerMetricStore();
    -	final Map<String, TaskManagerMetricStore> taskManagers = new HashMap<>();
    -	final Map<String, JobMetricStore> jobs = new HashMap<>();
    +	private final ComponentMetricStore jobManager = new ComponentMetricStore();
    +	private final Map<String, TaskManagerMetricStore> taskManagers = new ConcurrentHashMap<>();
    +	private final Map<String, JobMetricStore> jobs = new ConcurrentHashMap<>();
    +
    +	/**
    +	 * Remove not active task managers.
    +	 *
    +	 * @param activeTaskManagers to retain.
    +	 */
    +	public synchronized void retainTaskManagers(List<String> activeTaskManagers) {
    --- End diff --
    
    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.


---

Mime
View raw message