mirror of https://github.com/apache/druid.git
Add the pull-request template (#7206)
* Add the pull-request template * Rewording * Replaced checklist link, added Rat exclusion * Update the PR template. Add Concurrency Checklist to the repository * Merge Description and Design sections. Softer language. Removed requirement to test in production environment. Added a committer's instruction to justify addition of meta tags. * Rephrase item about comments * Add license header * Add item to concurrency checklist
This commit is contained in:
Roman Leventov
GitHub
parent
151edeec3c
commit
46ea5b88b7
|
|
@ -0,0 +1,65 @@
|
|||
Fixes #XXXX.
|
||||
|
||||
(Replace XXXX with the id of the issue fixed in this PR. Remove the above line if there is no corresponding
|
||||
issue. Don't reference the issue in the title of this pull-request.)
|
||||
|
||||
(If you are a committer, follow the PR action item checklist for committers:
|
||||
https://github.com/apache/incubator-druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers.)
|
||||
|
||||
### Description
|
||||
|
||||
Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's
|
||||
not necessary to repeat the description here, however, you may choose to keep one summary sentence.
|
||||
|
||||
Describe your patch: what did you change in code? How did you fix the problem?
|
||||
|
||||
If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For
|
||||
example:
|
||||
#### Fixed the bug ...
|
||||
#### Renamed the class ...
|
||||
#### Added a forbidden-apis entry ...
|
||||
|
||||
In each section, please describe design decisions made, including:
|
||||
- Choice of algorithms
|
||||
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such
|
||||
as when there are insufficient resources?
|
||||
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
|
||||
- Method organization and design (how the logic is split between methods, parameters and return types)
|
||||
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
|
||||
|
||||
It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point
|
||||
and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the
|
||||
advantages of the chosen designs and names.
|
||||
|
||||
If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any
|
||||
other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain
|
||||
what have changed in your final design compared to your original proposal or the consensus version in the end of the
|
||||
discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those
|
||||
aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description.
|
||||
|
||||
Some of the aspects mentioned above may be omitted for simple and small changes.
|
||||
|
||||
<hr>
|
||||
|
||||
This PR has:
|
||||
- [ ] been self-reviewed.
|
||||
- [ ] using the [concurrency checklist](
|
||||
https://github.com/apache/incubator-druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR
|
||||
doesn't have any relation to concurrency.)
|
||||
- [ ] added documentation for new or modified features or behaviors.
|
||||
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
|
||||
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar
|
||||
reader.
|
||||
- [ ] added unit tests or modified existing tests to cover new code paths.
|
||||
- [ ] added integration tests.
|
||||
- [ ] been tested in a test Druid cluster.
|
||||
|
||||
Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the
|
||||
items which are not done or not relevant to the PR. None of the items from the checklist above are strictly necessary,
|
||||
but it would be very helpful if you at least self-review the PR.
|
||||
|
||||
<hr>
|
||||
|
||||
For reviewers: the key changed/added classes in this PR are `MyFoo`, `OurBar`, and `TheirBaz`.
|
||||
|
||||
(Add this section in big PRs to ease navigation in them for reviewers.)
|
||||
|
|
@ -0,0 +1,199 @@
|
|||
<!--
|
||||
~ Licensed to the Apache Software Foundation (ASF) under one
|
||||
~ or more contributor license agreements. See the NOTICE file
|
||||
~ distributed with this work for additional information
|
||||
~ regarding copyright ownership. The ASF licenses this file
|
||||
~ to you under the Apache License, Version 2.0 (the
|
||||
~ "License"); you may not use this file except in compliance
|
||||
~ with the License. You may obtain a copy of the License at
|
||||
~
|
||||
~ http://www.apache.org/licenses/LICENSE-2.0
|
||||
~
|
||||
~ Unless required by applicable law or agreed to in writing,
|
||||
~ software distributed under the License is distributed on an
|
||||
~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
|
||||
~ KIND, either express or implied. See the License for the
|
||||
~ specific language governing permissions and limitations
|
||||
~ under the License.
|
||||
-->
|
||||
|
||||
## 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)
|
||||
- [Boundaries of non-blocking or benignly racy code are identified by WARNING comments?](
|
||||
https://github.com/code-review-checklists/java-concurrency#non-blocking-warning)
|
||||
|
||||
Threads and Executors
|
||||
- [Thread is named?](https://github.com/code-review-checklists/java-concurrency#name-threads)
|
||||
- [Thread is daemon?](#daemon-threads)
|
||||
- [Using `Execs` to create an `ExecutorService`?](#use-execs)
|
||||
- [Can use `ExecutorService` instead of creating a new `Thread` each time some method is called?](
|
||||
https://github.com/code-review-checklists/java-concurrency#reuse-threads)
|
||||
- [No network I/O in a CachedThreadPool?](
|
||||
https://github.com/code-review-checklists/java-concurrency#cached-thread-pool-no-io)
|
||||
- [No blocking (incl. I/O) operations in a `ForkJoinPool` or in a parallel Stream pipeline?
|
||||
](https://github.com/code-review-checklists/java-concurrency#fjp-no-blocking)
|
||||
- [Can execute non-blocking computation in `FJP.commonPool()` instead of a custom thread pool?](
|
||||
https://github.com/code-review-checklists/java-concurrency#use-common-fjp)
|
||||
|
||||
Parallel Streams
|
||||
- [Parallel Stream computation takes more than 100us in total?](
|
||||
https://github.com/code-review-checklists/java-concurrency#justify-parallel-stream-use)
|
||||
- [Comment before a parallel Streams pipeline explains how it takes more than 100us in total?](
|
||||
https://github.com/code-review-checklists/java-concurrency#justify-parallel-stream-use)
|
||||
|
||||
Thread interruption and `Future` cancellation
|
||||
- [Interruption status is restored before propagating a wrapped `InterruptedException`?
|
||||
](https://github.com/code-review-checklists/java-concurrency#restore-interruption)
|
||||
- [`InterruptedException` is swallowed only in the following kinds of methods?
|
||||
](https://github.com/code-review-checklists/java-concurrency#interruption-swallowing)
|
||||
- `Runnable.run()`, `Callable.call()`, or methods to be passed to executors as lambda tasks
|
||||
- Methods with "try" or "best effort" semantics
|
||||
- [`InterruptedException` swallowing is documented for a method?](
|
||||
https://github.com/code-review-checklists/java-concurrency#interruption-swallowing)
|
||||
- [Can use Guava's `Uninterruptibles` to avoid `InterruptedException` swallowing?](
|
||||
https://github.com/code-review-checklists/java-concurrency#interruption-swallowing)
|
||||
- [`Future` is canceled upon catching an `InterruptedException` or a `TimeoutException` on `get()`?](
|
||||
https://github.com/code-review-checklists/java-concurrency#cancel-future)
|
||||
|
||||
Time
|
||||
- [`nanoTime()` values are compared in an overflow-aware manner?](
|
||||
https://github.com/code-review-checklists/java-concurrency#nano-time-overflow)
|
||||
- [`currentTimeMillis()` is *not* used to measure time intervals and timeouts?](
|
||||
https://github.com/code-review-checklists/java-concurrency#time-going-backward)
|
||||
- [Units for a time variable are identified in the variable's name or via `TimeUnit`?](
|
||||
https://github.com/code-review-checklists/java-concurrency#time-units)
|
||||
- [Negative timeouts and delays are treated as zeros?](
|
||||
https://github.com/code-review-checklists/java-concurrency#treat-negative-timeout-as-zero)
|
||||
|
||||
Thread safety of Cleaners and native code
|
||||
- [`close()` is concurrently idempotent in a class with a `Cleaner` or `finalize()`?](
|
||||
https://github.com/code-review-checklists/java-concurrency#thread-safe-close-with-cleaner)
|
||||
- [Method accessing native state calls `reachabilityFence()` in a class with a `Cleaner` or `finalize()`?](
|
||||
https://github.com/code-review-checklists/java-concurrency#reachability-fence)
|
||||
- [`Cleaner` or `finalize()` is used for real cleanup, not mere reporting?](
|
||||
https://github.com/code-review-checklists/java-concurrency#finalize-misuse)
|
||||
- [Considered making a class with native state thread-safe?](
|
||||
https://github.com/code-review-checklists/java-concurrency#thread-safe-native)
|
||||
|
||||
<hr>
|
||||
|
||||
<a name="use-lifecycle-lock"></a>
|
||||
[#](#use-lifecycle-lock) Lk.D1. Is it possible to use Druid's `LifecycleLock` utility instead of a standard lock and
|
||||
"started" flag in lifecycled objects with `start()` and `stop()` methods? See the Javadoc comment for `LifecycleLock`
|
||||
for more details.
|
||||
|
||||
<a name="daemon-threads"></a>
|
||||
[#](#daemon-threads) TE.D1. Are Threads created directly or via a `ThreadFactory` configured to be daemon via
|
||||
`setDaemon(true)`? Note that by default, ThreadFactories constructed via `Execs.makeThreadFactory()` methods create
|
||||
daemon threads already.
|
||||
|
||||
<a name="use-execs"></a>
|
||||
[#](#use-execs) TE.D2. Is it possible to use one of the static factory methods in Druid's `Execs` utility class to
|
||||
create an `ExecutorService` instead of Java's standard `ExecutorServices`? This is recommended because `Execs` configure
|
||||
ThreadFactories to create daemon threads by default, as required by the previous item.
|
||||
|
|
@ -87,7 +87,10 @@ committer who visits an issue or a PR authored by a non-committer.
|
|||
that need to be merged before some other PRs could even be published. `Development Blocker` PRs should be prioritized
|
||||
by reviewers, so that they could be merged as soon as possible, thus not blocking somebody's work.
|
||||
|
||||
2. Consider adding one or several **`Area -`** tags to the PR or issue. Consider [creating a new `Area -` tag](
|
||||
2. If you added some tags on the previous step, describe why did you do that, either in the PR description (if you are
|
||||
the author of the PR) or in a comment (if you have added tags to a PR submitted by someone else).
|
||||
|
||||
3. Consider adding one or several **`Area -`** tags to the PR or issue. Consider [creating a new `Area -` tag](
|
||||
#creating-a-new-tag-on-github) if none of the existing `Area` tags is applicable to the PR or issue.
|
||||
|
||||
- [`Area - Automation/Static Analysis`](
|
||||
|
|
@ -131,15 +134,15 @@ committer who visits an issue or a PR authored by a non-committer.
|
|||
any PRs and issues related to ZooKeeper, Curator, and node discovery in Druid.
|
||||
|
||||
|
||||
3. **Consider adding any `Bug` and `Security` PRs to the next Druid milestone** whenever they are important enough to
|
||||
4. **Consider adding any `Bug` and `Security` PRs to the next Druid milestone** whenever they are important enough to
|
||||
fix before the next release. This ensures that they will be considered by the next release manager as potential release
|
||||
blockers. Please don't add PRs that are neither `Bug` nor `Security`-related to milestones until after they are
|
||||
committed, to avoid cluttering the release manager's workflow.
|
||||
|
||||
4. If the PR has obvious problems, such as an empty description or the PR fails the CI, ask the PR author to fix these
|
||||
5. If the PR has obvious problems, such as an empty description or the PR fails the CI, ask the PR author to fix these
|
||||
problems even if you don't plan to review the PR.
|
||||
|
||||
5. If you create an issue that is relatively small and self-contained and you don't plan to work on it in the near
|
||||
6. If you create an issue that is relatively small and self-contained and you don't plan to work on it in the near
|
||||
future, consider tagging it [**`Contributions Welcome`**](
|
||||
https://github.com/apache/incubator-druid/labels/Contributions%20Welcome) so that other people know that the issue is
|
||||
free to pick up and is relatively easily doable even for those who are not very familiar with the codebase.
|
||||
|
|
|
|||
4
pom.xml
4
pom.xml
|
|
@ -1609,6 +1609,7 @@
|
|||
<exclude>NOTICE.BINARY</exclude>
|
||||
<exclude>LABELS</exclude>
|
||||
<exclude>.github/ISSUE_TEMPLATE/*.md</exclude>
|
||||
<exclude>.github/pull_request_template.md</exclude>
|
||||
<exclude>git.version</exclude>
|
||||
<exclude>node_modules/**</exclude>
|
||||
<exclude>coordinator-console/**</exclude>
|
||||
|
|
@ -1619,7 +1620,8 @@
|
|||
</plugins>
|
||||
</build>
|
||||
</profile>
|
||||
<!-- Prevent the source-release-assembly execution defined in the Apache parent POM from running so we can control it ourselves -->
|
||||
<!-- Prevent the source-release-assembly execution defined in the Apache parent POM from running
|
||||
so we can control it ourselves -->
|
||||
<profile>
|
||||
<id>apache-release</id>
|
||||
<build>
|
||||
|
|
|
|||
Loading…
Reference in New Issue