jon-wei commented on a change in pull request #7206: Add the pull-request template URL: https://github.com/apache/incubator-druid/pull/7206#discussion_r276880835
########## File path: dev/code-review/concurrency.md ########## @@ -0,0 +1,178 @@ +## Druid's Checklist for Concurrency Code + +Design + - [Concurrency is rationalized in the PR description?]( + https://github.com/code-review-checklists/java-concurrency#rationalize) + - [Can use patterns to simplify concurrency?](https://github.com/code-review-checklists/java-concurrency#use-patterns) + - Immutability/Snapshotting + - Divide and conquer + - Producer-consumer + - Instance confinement + - Thread/Task/Serial thread confinement + +Documentation + - [Thread-safety is justified in comments?]( + https://github.com/code-review-checklists/java-concurrency#justify-document) + - [Class (method, field) has concurrent access documentation?]( + https://github.com/code-review-checklists/java-concurrency#justify-document) + - [Threading model of a subsystem (class) is described?]( + https://github.com/code-review-checklists/java-concurrency#threading-flow-model) + - [Concurrent control flow (or data flow) of a subsystem (class) is described?]( + https://github.com/code-review-checklists/java-concurrency#threading-flow-model) + - [Class is documented as immutable, thread-safe, or not thread-safe?]( + https://github.com/code-review-checklists/java-concurrency#immutable-thread-safe) + - [Applied concurrency patterns are pronounced?]( + https://github.com/code-review-checklists/java-concurrency#name-patterns) + - [`@GuardedBy` annotation is used?](https://github.com/code-review-checklists/java-concurrency#guarded-by) + - [Safety of benign races is explained?]( + https://github.com/code-review-checklists/java-concurrency#document-benign-race) + - [Each use of `volatile` is justified?]( + https://github.com/code-review-checklists/java-concurrency#justify-volatile) + - [Field that is neither `volatile` nor annotated with `@GuardedBy` has a comment?]( + https://github.com/code-review-checklists/java-concurrency#plain-field) + +Excessive thread safety + - [No "extra" (pseudo) thread safety?](https://github.com/code-review-checklists/java-concurrency#pseudo-safety) + - [No atomics on which only `get()` and `set()` are called?]( + https://github.com/code-review-checklists/java-concurrency#redundant-atomics) + - [Class (method) needs to be thread-safe?]( + https://github.com/code-review-checklists/java-concurrency#unneeded-thread-safety) + +Race conditions + - [No `put()` or `remove()` calls on a `ConcurrentHashMap` after `get()` or `containsKey()`?]( + https://github.com/code-review-checklists/java-concurrency#chm-race) Review comment: I think the wording on some of the checklist items could be made clearer, e.g.: ``` # RC.1. Aren’t ConcurrentHashMaps updated with multiple separate containsKey(), get(), put() and remove() calls instead of a single call to compute()/computeIfAbsent()/computeIfPresent()/replace()? ``` From that it's somewhat ambiguous as to which one is the positive recommendation, I think would be clearer if phrased as: ``` # RC.1. ConcurrentHashMaps should be updated with a single call to compute()/computeIfAbsent()/computeIfPresent()/replace() instead of with multiple separate containsKey(), get(), put() and remove() calls ``` and for places with similar use of "aren't". ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
