This is an automated email from the ASF dual-hosted git repository.
sunithabeeram pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 63f0d3d readthedocs: Add contribution guidelines (#3925)
63f0d3d is described below
commit 63f0d3d386079b0ba63a880eaf84464742ba4814
Author: Sunitha Beeram <[email protected]>
AuthorDate: Thu Mar 7 12:00:21 2019 -0800
readthedocs: Add contribution guidelines (#3925)
* Add contribution guidelines
* Add info about travis
* Address review comments
---
docs/code_modules.rst | 19 ++++-
docs/contribution_guidelines.rst | 167 ++++++++++++++++++++++++++++++++++++---
2 files changed, 173 insertions(+), 13 deletions(-)
diff --git a/docs/code_modules.rst b/docs/code_modules.rst
index adb1fc7..6a01abf 100644
--- a/docs/code_modules.rst
+++ b/docs/code_modules.rst
@@ -59,6 +59,10 @@ overall executable size for individually deployable
component.
Each module has a ``src/main/java`` folder where the code resides and
``src/test/java`` where the *unit* tests corresponding to
the module's code reside.
+.. _pinot-foundation:
+
+Foundational modules
+--------------------
The following figure provides a high-level overview of the foundational Pinot
modules.
.. figure:: img/PinotFoundation.png
@@ -132,10 +136,21 @@ Auxiliary modules
-----------------
In addition to the core modules described above, Pinot code provides the
following modules:
-* ``pinot-tools``: This module is a collection of many tools useful for
setting up Pinot cluster, creating/updating segments. It also houses the Pinot
quick start guide code.
+* ``pinot-tools``: This module is a collection of many tools useful for
setting up Pinot cluster, creating/updating segments.
+It also houses the Pinot quick start guide code.
* ``pinot-perf``: This module has a collection of benchmark test code used to
evaluate design options.
* ``pinot-client-api``: This module houses the Java client API. See
:ref:`java-client` for more info.
-* ``pinot-integration-tests``: This module holds integration tests that test
functionality across multiple classes or components. These tests typically do
not rely on mocking and provide more end to end coverage for code.
+* ``pinot-integration-tests``: This module holds integration tests that test
functionality across multiple classes or components.
+These tests typically do not rely on mocking and provide more end to end
coverage for code.
+
+.. _extension-modules:
+
+Extension modules
+-----------------
+``pinot-hadoop-filesystem`` and ``pinot-azure-filesystem`` are module added to
support extensions to Pinot filesystem.
+The functionality is broken down into modules of their own to avoid polluting
the common modules with additional large libraries.
+These libraries bring in transitive dependencies of their own that can cause
classpath conflicts at runtime. We would like to
+avoid this for the common usage of Pinot as much as possible.
\ No newline at end of file
diff --git a/docs/contribution_guidelines.rst b/docs/contribution_guidelines.rst
index 0ac2f86..b1620c1 100644
--- a/docs/contribution_guidelines.rst
+++ b/docs/contribution_guidelines.rst
@@ -26,41 +26,186 @@ Create an issue for the change
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Create a Pinot issue for the change you would like to make. Provide
information on why the change is needed and how you
plan to address it. Use the conversations on the issue as a way to validate
assumptions and the right way to proceed.
+Be sure to review sections on :ref:`compatiblity-changes` and
:ref:`external-libs`.
-Once you have a good about what you want to do, proceed with the next steps.
+Once you are clear about what you want to do, proceed with the next steps
listed below.
Create a branch for your change
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+.. code-block:: none
+
+ $ cd incubator-pinot
+ $ git checkout -b <your issue branch>
+
+Make the necessary changes. If the changes you plan to make are too big, make
sure you break it down into smaller tasks.
+
+Code documentation
+^^^^^^^^^^^^^^^^^^
+Please ensure your code is adequately documented. Some things to consider for
documentation:
+
+* Always include class level java docs.
+At the top class level, we are looking for information about what
functionality is provided by the class,
+what state is maintained by the class, whether there are
concurrency/thread-safety concerns and any exceptional behavior that the class
might exhibit.
+
+* Document public methods and their parameters.
+
+Logging
+^^^^^^^
+
+* Ensure there is adequate logging for positive paths as well as exceptional
paths. As a corollary to this, ensure logs are not noisy.
+* Do not use System.out.println to log messages. Use the ``slf4j`` loggers.
+* Use logging levels correctly: set level to ``debug`` for verbose logs useful
for only for debugging.
+* Do not log stack traces via ``printStackTrace`` method of the exception.
+
+Exceptions and Exception-Handling
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+* Where possible, throw specific exceptions, preferably checked exceptions, so
the callers can easily determine what the erroneous conditions that need to be
handled are.
+* Avoid catching broad exceptions (ie, ``catch (Exception e)`` blocks, except
for when this is in the run() method of a thread/runnable.
+
+Current Pinot code does not strictly adhere to this, but we would like to
change this over time and adopt best practices around exception handling.
+
+.. _compatibility-changes:
+
+Backward and Forward compatibility changes
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+If you are making any changes to state stored, either in Zookeeper or in
segments, make sure you consider both backward and forward compatibility issues.
+
+* For backward compatibility, consider cases where one component is using the
new version and another is still on the old version. E.g., when the request
format between broker and server is updated, consider resulting behaviors when
a new broker is talking to an older server. Will it break?
+* For forward compatibility, consider rollback cases. E.g., consider what
happens when state persisted by new code is handled by old code. Does the old
code skip over new fields?
+
+.. _external-libs:
+
+External libraries
+^^^^^^^^^^^^^^^^^^
+Be cautious about pulling in external dependencies. You will need to consider
multiple things when faced with a need to pull in a new library.
+
+* What capability is the addition of the library providing you with? Can
existing libraries provide this functionality (may be with a little bit of
effort)?
+* Is the external library maintained by an active community of contributors?
+* What are the licensing terms for the library. For more information about
handling licenses, see :ref:`handling-licenses`.
+* Are you adding the library to :ref:`pinot-foundation` modules? This will
affect the rest of the Pinot code base.
+If the new library pulls in a lot of transitive dependencies, then we might
encounter unexpected issues with multiple classes in the classpath.
+These issues are hard to catch with tests as the order of loading the
libraries at runtime matters. If you absolutely need the support, consider
adding it via extension modules, see :ref:`extension-modules`.
+
+Testing your changes
+^^^^^^^^^^^^^^^^^^^^
+Identify a list of tests for the changes you have made. Depending on the scope
of changes, you may need one or more of the following tests:
+
+* Unit Tests
+
+ Make sure your code has the necessary class or method level unit tests. It
is important to write both positive case as well as negative case tests.
+ Document your tests well and add meaningful assertions in the tests; when
the assertions fail, ensure that the right messages are logged with information
that allows other to debug.
+
+* Mocking
+
+ Use `Mockito <https://site.mockito.org/>`_ to mock classes to control
specific behaviors - e.g., simulate various error conditions.
+
+.. note::
+ DO NOT use advanced mock libraries such as `PowerMock
<https://github.com/powermock/powermock>`_. They make bytecode level changes to
allow tests for static/private members but this typically results in other
tools like jacoco to fail. They also promote incorrect implementation choices
that make it harder to test additional changes. When faced with a choice to use
PowerMock or advanced mocking options, you might either need to refactor the
code to work better with mocking or you actual [...]
+
+* Validate assumptions in tests
+
+ Make sure that adequate asserts are added in the tests to verify that the
tests are passing for the right reasons.
+
+* Write reliable tests
+
+ Make sure you are writing tests that are reliable. If the tests depend on
asynchronous events to be fired, do not add ``sleep`` to your tests. Where
possible, use appropriate mocking or condition based triggers.
+
+.. _handling-licenses:
+
+License Headers for newly added files
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+All source code files should have license headers. To automatically add the
header for any new file you plan to checkin, run in ``incubator-pinot``
top-level folder:
+
+.. code-block:: none
+
+ mvn license:format
+
+.. note::
+
+If you checkin third-party code or files, please make sure you review Apache
guidelines:
+
+* `Licences that can be included
<https://www.apache.org/legal/resolved.html#what-can-we-include-in-an-asf-project-category-a>`_
+
+* `Licences that may be included
<https://www.apache.org/legal/resolved.html#what-can-we-maybe-include-in-an-asf-project-category-b>`_
+
+* `Licenses that should not be included
<https://www.apache.org/legal/resolved.html#what-can-we-not-include-in-an-asf-project-category-x>`_
+
+Once you determine the code you are pulling in adhere to the guidelines above,
go ahead pull the changes in.
+Do not add license headers for them. Follow these instructions to ensure we
are compliant with Apache Licensing process:
+
+* Under ``incubator-pinot/licenses`` add a LICENSE-<newlib> file that has the
license terms of the included library.
+* Update the ``incubator-pinot/LICENSE`` file to indicate the newly added
library file paths under the corresponding supported Licenses.
+* Update the exclusion rules for ``license`` and ``rat`` maven plugins in the
parent pom: ``incubator-pinot/pom.xml``.
+
+If attention to the licensing terms in not paid early on, they will be caught
much later in the process, when we prepare to make a new release.
+Updating code at that time to work with the right libraries at that time might
require bigger refactoring changes and delay the release process.
+
Creating a PR
^^^^^^^^^^^^^
* Verifying code-style
-
Run the following command to verify the code-style before posting a PR
.. code-block:: none
mvn checkstyle:check
-* License Headers for newly added files
-All source code files should have license headers. To automatically add the
header for any new file you plan to checkin,
- run:
+* Run tests
+
+Before you create a review request for the changes, make sure you have run the
corresponding unit tests for your changes.
+You can run individual tests via the IDE or via maven command-line. Finally
run all tests locally by running ``mvn clean install -Pbin-dist``.
+
+For changes that are related to performance issues or race conditions, it is
hard to write reliable tests, so we recommend running manual stress tests to
validate the changes. You ``MUST`` note the manual tests done in the PR
description.
+
+* Push changes and create a PR for review
+
+Commit your changes with a meaningful commit message.
.. code-block:: none
- mvn license:format
+ $ git add <files required for the change>
+ $ git commit -m "Meaningful oneliner for the change"
+ $ git push origin <your issue branch>
-.. note::
+After this, create a PullRequest in `github
<https://github.com/apache/incubator-pinot/pulls>`_. Include the following
information in the description:
-If you checkin third-party code or files, do not add license headers for them.
Follow these instructions to ensure we
-are compliant with Apache Licensing process. <TBD>
+* The changes that are included in the PR.
-* Run tests
+* Information on any implementation choices that were made.
-* Push changes and create a PR for review
+* Evidence of sufficient testing. You ``MUST`` indicate the tests done, either
manually or automated.
+
+Once the PR is created, the code base is compiled and all tests are run via
``travis``. Make sure you followup on any issues flagged by travis and address
them.
+If you see test failures that are intermittent, ``please`` create an issue to
track them.
+
+Once the ``travis`` run is clear, request reviews from atleast 2 committers on
the project and be sure to gently to followup on the issue with the reviewers.
* Addressing comments and Rebasing
+Once you receive comments on github on your changes, be sure to respond to
them on github and address the concerns.
+If any discussions happen offline for the changes in question, make sure to
capture the outcome of the discussion, so others can follow along as well.
+
+It is possible that while your change is being reviewed, other changes were
made to the master branch. Be sure to pull rebase your change on the new
changes thus:
+
+.. code-block:: none
+
+ # commit your changes
+ $ git add <updated files>
+ $ git commit -m "Meaningful message for the udpate"
+ # pull new changes
+ $ git checkout master
+ $ git pull
+ $ git checkout <your issue branch>
+ $ git rebase master
+
+At this time, if rebase flags any conflicts, resolve the conflicts and follow
the instructions provided by the rebase command.
+
+Run additional tests/validations for the new changes and update the PR by
pushing your changes:
+
+.. code-block:: none
+ $ git push origin <your issue branch>
* Once your change is merged, check to see if any documentation needs to be
updated. If so, create a PR for documentation.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
