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_r277059714
########## 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) + - [No point accesses to a non-thread-safe collection outside of critical sections?]( + https://github.com/code-review-checklists/java-concurrency#unsafe-concurrent-point-read) + - [Iteration over a non-thread-safe collection doesn't leak outside of a critical section?]( + https://github.com/code-review-checklists/java-concurrency#unsafe-concurrent-iteration) + - [Non-trivial object is *not* returned from a getter in a thread-safe class?]( + https://github.com/code-review-checklists/java-concurrency#concurrent-mutation-race) + - [No separate getters to an atomically updated state?]( + https://github.com/code-review-checklists/java-concurrency#moving-state-race) + - [No state used for making decisions or preparing data inside a critical section is read outside?]( + https://github.com/code-review-checklists/java-concurrency#read-outside-critical-section-race) + - [No race conditions are possible between the program and users or other programs?]( + https://github.com/code-review-checklists/java-concurrency#outside-world-race) + - [No race conditions are possible on the file system?]( + https://github.com/code-review-checklists/java-concurrency#outside-world-race) + +Replacing locks with concurrency utilities + - [Can use `LifecycleLock` instead of a standard lock in a lifecycled object?](#use-lifecycle-lock) + - [Can use concurrency utility instead of `Object.wait()`/`notify()`?]( + https://github.com/code-review-checklists/java-concurrency#avoid-wait-notify) + - [Can use Guava’s `Monitor` instead of a standard lock with conditional waits?]( + https://github.com/code-review-checklists/java-concurrency#guava-monitor) + +Avoiding deadlocks + - [Can avoid nested critical sections?]( + https://github.com/code-review-checklists/java-concurrency#avoid-nested-critical-sections) + - [Locking order for nested critical sections is documented?]( + https://github.com/code-review-checklists/java-concurrency#document-locking-order) + - [Dynamically determined locks for nested critical sections are ordered?]( + https://github.com/code-review-checklists/java-concurrency#dynamic-lock-ordering) + - [No extension API calls within critical sections?]( + https://github.com/code-review-checklists/java-concurrency#non-open-call) + +Improving scalability + - [Critical section is as small as possible?]( + https://github.com/code-review-checklists/java-concurrency#minimize-critical-sections) + - [Can use `ConcurrentHashMap.compute()` or Guava's `Striped` for per-key locking?]( + https://github.com/code-review-checklists/java-concurrency#increase-locking-granularity) + - [Can replace blocking collection or a queue with a concurrent one?]( + https://github.com/code-review-checklists/java-concurrency#non-blocking-collections) + - [Can use `ClassValue` instead of `ConcurrentHashMap<Class, ...>`?]( + https://github.com/code-review-checklists/java-concurrency#use-class-value) + - [Considered `ReadWriteLock` (or `StampedLock`) instead of a simple lock?]( + https://github.com/code-review-checklists/java-concurrency#read-write-lock) + - [`StampedLock` is used instead of `ReadWriteLock` when reentrancy is not needed?]( + https://github.com/code-review-checklists/java-concurrency#use-stamped-lock) + - [Considered `LongAdder` instead of an `AtomicLong` for a "hot field"?]( + https://github.com/code-review-checklists/java-concurrency#long-adder-for-hot-fields) + +Lazy initialization and double-checked locking + - [Lazy initialization of a field should be thread-safe?]( + https://github.com/code-review-checklists/java-concurrency#lazy-init-thread-safety) + - [Considered double-checked locking for a lazy initialization to improve performance?]( + https://github.com/code-review-checklists/java-concurrency#use-dcl) + - [Considered eager initialization instead of a lazy initialization to simplify code?]( + https://github.com/code-review-checklists/java-concurrency#eager-init) + - [Double-checked locking follows the SafeLocalDCL pattern?]( + https://github.com/code-review-checklists/java-concurrency#safe-local-dcl) + - [Can do lazy initialization with a benign race and without locking to improve performance?]( + https://github.com/code-review-checklists/java-concurrency#lazy-init-benign-race) + +Non-blocking and partially blocking code + - [Non-blocking code has enough comments to make line-by-line checking as easy as possible?]( + https://github.com/code-review-checklists/java-concurrency#check-non-blocking-code) + - [Can use immutable POJO + compare-and-swap operations to simplify non-blocking code?]( + https://github.com/code-review-checklists/java-concurrency#swap-state-atomically) + +Threads and Executors + - [Thread is named?](https://github.com/code-review-checklists/java-concurrency#name-threads) + - [Thread is daemon?](#daemon-threads) Review comment: ah, got it ---------------------------------------------------------------- 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]
