flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthias J. Sax" <mj...@apache.org>
Subject Re: Extending and improving our "How to contribute" page
Date Mon, 05 Oct 2015 14:34:18 GMT
I would like to extend the coding guidelines to make new tests (or
extending existing once) for fixed bugs mandatory; ie, write a test that
fails before the fix, but passes after the fix.

Right now, the guideline dictates that **Tests for new features are
required** which is not broad enough, IMHO.

What do you think?

-Matthias

On 10/05/2015 02:09 PM, Fabian Hueske wrote:
> Hi,
> 
> I opened a PR with the discussed changes [1].
> Please review, give feedback, and suggest changes.
> 
> Cheers, Fabian
> 
> [1] https://github.com/apache/flink-web/pull/11
> 
> 
> 2015-09-28 18:02 GMT+02:00 Fabian Hueske <fhueske@gmail.com>:
> 
>> @Chiwan, sure. Will do that. Thanks for pointing it out :-)
>>
>> 2015-09-28 18:00 GMT+02:00 Chiwan Park <chiwanpark@apache.org>:
>>
>>> @Fabian, Could you cover FLINK-2712 in your pull request? I think that it
>>> would be better than split pull request.
>>>
>>> Regards,
>>> Chiwan Park
>>>
>>>> On Sep 28, 2015, at 4:51 PM, Fabian Hueske <fhueske@gmail.com> wrote:
>>>>
>>>> Thanks everybody for the discussion.
>>>> I'll prepare a pull request to update the "How to contribute" and
>>> "Coding
>>>> Guidelines".
>>>>
>>>> Thanks,
>>>> Fabian
>>>>
>>>> 2015-09-26 9:06 GMT+02:00 Maximilian Michels <mxm@apache.org>:
>>>>
>>>>> Hi Fabian,
>>>>>
>>>>> This is a very important topic. Thanks for starting the discussion.
>>>>>
>>>>> 1) JIRA discussion
>>>>>
>>>>> Absolutely. No new feature should be introduced without a discussion.
>>>>> Frankly, I see the problem that sometimes discussions only come up
>>>>> when the pull request has been opened. However, this can be overcome
>>>>> by the design document.
>>>>>
>>>>> 2) Design document
>>>>>
>>>>> +1 for the document. It increases transparency but also helps the
>>>>> contributor to think his idea through before starting to code. The
>>>>> document could also be written directly in JIRA. That way, it is more
>>>>> accessible. JIRA offers mark up; even images can be attached and
>>>>> displayed in the JIRA description.
>>>>>
>>>>> I'd like to propose another section "Limitations" for the design
>>>>> document. Breaking API changes should also be listed on a special Wiki
>>>>> page.
>>>>>
>>>>> 3) Coding style
>>>>>
>>>>> In addition to updating the document, do we want to enforce coding
>>>>> styles also by adding new Maven Checkstyle rules? IMHO strict rules
>>>>> could cause more annoyances than they actually contribute to the
>>>>> readability of the code. Perhaps this should be discussed in a
>>>>> separate thread.
>>>>>
>>>>> +1 for collecting common problems and design patterns to include them
>>>>> in the document. I was thinking, that we should also cover some of the
>>>>> features of tools and dependencies we heavily use, e.g. Travis,
>>>>> Mockito, Guava, Log4j, FlinkMiniCluster, Unit testing vs IT cases,
>>>>> etc.
>>>>>
>>>>> 4 ) Restructuring the how to contribute guide
>>>>>
>>>>> Good idea to have a meta document that explains how contributing works
>>>>> in general, and another document for technical things.
>>>>>
>>>>>
>>>>> Cheers,
>>>>> Max
>>>>>
>>>>>
>>>>> On Thu, Sep 24, 2015 at 2:53 PM, Fabian Hueske <fhueske@gmail.com>
>>> wrote:
>>>>>>
>>>>>> Thanks everybody for feedback and comments.
>>>>>>
>>>>>> Regarding 1) and 2):
>>>>>>
>>>>>> I like the idea of keeping the discussion of new features and
>>>>> improvements
>>>>>> in JIRA as Kostas proposed.
>>>>>> Our coding guidelines [1] already request a JIRA issue for each pull
>>>>>> request.
>>>>>>
>>>>>> How about we highlight this requirement more prominently and follow
>>> this
>>>>>> rule more strict from now on.
>>>>>> JIRA issues for new features and improvements should clearly specify
>>> the
>>>>>> scope and requirements for the new feature / improvement.
>>>>>> The level of detail is up to the reporter of the issue, but the
>>> community
>>>>>> can request more detail or change the scope and requirements by
>>>>> discussion.
>>>>>> When a JIRA issue for a new feature or improvement is opened, the
>>>>> community
>>>>>> can start a discussion whether the feature is desirable for Flink
or
>>> not.
>>>>>> Any contributor (including the reporter) can also attach a
>>>>>> "design-doc-requested" label to the issue. A design document can
be
>>>>>> proposed by anybody, including the reporter or assignee of the JIRA
>>>>> issue.
>>>>>> However, the issue cannot be resolved and a corresponding PR not
be
>>>>> merged
>>>>>> before a design document has been accepted by lazy consensus. Hence,
>>> an
>>>>>> assignee should propose a design doc before starting to code to avoid
>>>>> major
>>>>>> redesigns of the implementation.
>>>>>>
>>>>>> This way it is up to the community when to start a discussion about
>>>>> whether
>>>>>> a feature request is accepted or to request a design document. We
can
>>>>> make
>>>>>> design documents mandatory for changes that touch the public API.
>>>>>>
>>>>>> Regarding 3):
>>>>>>
>>>>>> I agree with Vasia, that we should collect suggestions for common
>>>>> patterns
>>>>>> and also continuously update the coding guidelines.
>>>>>> @Henry, I had best practices (exception handling, tests, etc.) in
>>> mind.
>>>>>> Syntactic code style is important as well, but we should have a
>>> separate
>>>>>> discussion about that, IMO.
>>>>>>
>>>>>> Proposal for a design document template:
>>>>>>
>>>>>> - Overview of general approach
>>>>>> - API changes (changed interfaces, new / deprecated configuration
>>>>>> parameters, changed behavior)
>>>>>> - Main components and classes to touch
>>>>>>
>>>>>> Cheers,
>>>>>> Fabian
>>>>>>
>>>>>> [1] http://flink.apache.org/coding-guidelines.html
>>>>>> <http://flink.apache.org/coding-guidelines.html>
>>>>>>
>>>>>> 2015-09-24 10:52 GMT+02:00 Chiwan Park <chiwanpark@apache.org>:
>>>>>>
>>>>>>> Thanks Fabian for starting the discussion.
>>>>>>>
>>>>>>> +1 for overall approach.
>>>>>>>
>>>>>>> About (1), expressing that consensus must be required for new
feature
>>>>> in
>>>>>>> “How to contribute” page is very nice. Some pull requests
were sent
>>>>> without
>>>>>>> consensus. The contributors had to rewrote their pull requests.
>>>>>>>
>>>>>>> Agree with (2), (3) and (4).
>>>>>>>
>>>>>>> Regards,
>>>>>>> Chiwan Park
>>>>>>>
>>>>>>>> On Sep 24, 2015, at 2:23 AM, Henry Saputra <henry.saputra@gmail.com
>>>>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Thanks again, Fabian for starting the discussions.
>>>>>>>>
>>>>>>>> For (1) and (2) I think it is good idea and will help people
to
>>>>>>>> understand and follow the author thought process.
>>>>>>>> Following up with Stephan's reply, some new features solutions
could
>>>>>>>> be explained thoroughly in the PR descriptions but some requires
>>>>>>>> additional reviews of the proposed design.
>>>>>>>> I like the idea of using tag in JIRA whether new features
should or
>>>>>>>> should not being accompanied by design document.
>>>>>>>>
>>>>>>>> Agree with (3) and (4).
>>>>>>>> As for (3) are you thinking about more of style of code syntax
via
>>>>>>>> checkstyle updates, or best practices in term of no mutable
state if
>>>>>>>> possible, throw precise Exception if possible for interfaces,
etc. ?
>>>>>>>>
>>>>>>>> - Henry
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, Sep 23, 2015 at 9:31 AM, Stephan Ewen <sewen@apache.org>
>>>>> wrote:
>>>>>>>>> Thanks, Fabian for driving this!
>>>>>>>>>
>>>>>>>>> I agree with your points.
>>>>>>>>>
>>>>>>>>> Concerning Vasia's comment to not raise the bar too high:
>>>>>>>>> That is true, the requirements should be reasonable.
We can
>>>>> definitely
>>>>>>> tag
>>>>>>>>> issues as "simple" which means they do not require a
design
>>>>> document.
>>>>>>> That
>>>>>>>>> should be more for new features and needs not be very
detailed.
>>>>>>>>>
>>>>>>>>> We could also make the inverse, meaning we explicitly
tag certain
>>>>>>> issues as
>>>>>>>>> "requires design document".
>>>>>>>>>
>>>>>>>>> Greetings,
>>>>>>>>> Stephan
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Sep 23, 2015 at 5:05 PM, Vasiliki Kalavri <
>>>>>>> vasilikikalavri@gmail.com
>>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> I agree with you Fabian. Clarifying these issues
in the "How to
>>>>>>> Contribute"
>>>>>>>>>> guide will save lots of time both to reviewers and
contributors.
>>>>> It is
>>>>>>> a
>>>>>>>>>> really disappointing situation when someone spends
time
>>>>> implementing
>>>>>>>>>> something and their PR ends up being rejected because
either the
>>>>>>> feature
>>>>>>>>>> was not needed or the implementation details were
never agreed on.
>>>>>>>>>>
>>>>>>>>>> That said, I think we should also make sure that
we don't raise
>>> the
>>>>>>> bar too
>>>>>>>>>> high for simple contributions.
>>>>>>>>>>
>>>>>>>>>> Regarding (1) and (2), I think we should clarify
what kind of
>>>>>>>>>> additions/changes require this process to be followed.
e.g. do we
>>>>> need
>>>>>>> to
>>>>>>>>>> discuss additions for which JIRAs already exist?
Ideas described
>>>>> in the
>>>>>>>>>> roadmaps? Adding a new algorithm to Gelly/Flink-ML?
>>>>>>>>>>
>>>>>>>>>> Regarding (3), maybe we can all suggest some examples/patterns
>>> that
>>>>>>> we've
>>>>>>>>>> seen when reviewing PRs and then choose the most
common (or all).
>>>>>>>>>>
>>>>>>>>>> (4) sounds good to me.
>>>>>>>>>>
>>>>>>>>>> Cheers,
>>>>>>>>>> Vasia.
>>>>>>>>>>
>>>>>>>>>> On 23 September 2015 at 15:08, Kostas Tzoumas <
>>> ktzoumas@apache.org
>>>>>>
>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Big +1.
>>>>>>>>>>>
>>>>>>>>>>> For (1), a discussion in JIRA would also be an
option IMO
>>>>>>>>>>>
>>>>>>>>>>> For (2), let us come up with few examples on
what constitutes a
>>>>>>> feature
>>>>>>>>>>> that needs a design doc, and what should be in
the doc (IMO
>>>>>>>>>>> architecture/general approach, components touched,
interfaces
>>>>> changed)
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Sep 23, 2015 at 2:24 PM, Fabian Hueske
<
>>> fhueske@gmail.com
>>>>>>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi everybody,
>>>>>>>>>>>>
>>>>>>>>>>>> I guess we all have noticed that the Flink
community is quickly
>>>>>>> growing
>>>>>>>>>>> and
>>>>>>>>>>>> more and more contributions are coming in.
Recently, a few
>>>>>>>>>> contributions
>>>>>>>>>>>> proposed new features without being discussed
on the mailing
>>>>> list.
>>>>>>> Some
>>>>>>>>>>> of
>>>>>>>>>>>> these contributions were not accepted in
the end. In other
>>> cases,
>>>>>>> pull
>>>>>>>>>>>> requests had to be heavily reworked because
the approach taken
>>>>> was
>>>>>>> not
>>>>>>>>>>> the
>>>>>>>>>>>> best one. These are situations which should
be avoided because
>>>>> both
>>>>>>> the
>>>>>>>>>>>> contributor as well as the person who reviewed
the contribution
>>>>>>>>>> invested
>>>>>>>>>>> a
>>>>>>>>>>>> lot of time for nothing.
>>>>>>>>>>>>
>>>>>>>>>>>> I had a look at our “How to contribute”
and “Coding guideline”
>>>>> pages
>>>>>>>>>> and
>>>>>>>>>>>> think, we can improve them. I see basically
two issues:
>>>>>>>>>>>>
>>>>>>>>>>>> 1. The documents do not explain how to propose
and discuss new
>>>>>>>>>> features
>>>>>>>>>>>> and improvements.
>>>>>>>>>>>> 2. The documents are quite technical and
the structure could be
>>>>>>>>>>> improved,
>>>>>>>>>>>> IMO.
>>>>>>>>>>>>
>>>>>>>>>>>> I would like to improve these pages and propose
the following
>>>>>>>>>> additions:
>>>>>>>>>>>>
>>>>>>>>>>>> 1. Request contributors and committers to
start discussions on
>>>>> the
>>>>>>>>>>>> mailing list for new features. This discussion
should help to
>>>>> figure
>>>>>>>>>> out
>>>>>>>>>>>> whether such a new feature is a good fit
for Flink and give
>>> first
>>>>>>>>>>> pointers
>>>>>>>>>>>> for a design to implement it.
>>>>>>>>>>>> 2. Require contributors and committers to
write design
>>>>> documents for
>>>>>>>>>>> all
>>>>>>>>>>>> new features and major improvements. These
documents should be
>>>>>>> attached
>>>>>>>>>>> to
>>>>>>>>>>>> a JIRA issue and follow a template which
needs to be defined.
>>>>>>>>>>>> 3. Extend the “Coding Style Guides” and
add patterns that are
>>>>>>>>>> commonly
>>>>>>>>>>>> remarked in pull requests.
>>>>>>>>>>>> 4. Restructure the current pages into three
pages: a general
>>>>> guide
>>>>>>>>>> for
>>>>>>>>>>>> contributions and two guides for how to contribute
to code and
>>>>>>> website
>>>>>>>>>>> with
>>>>>>>>>>>> all technical issues (repository, IDE setup,
build system, etc.)
>>>>>>>>>>>>
>>>>>>>>>>>> Looking forward for your comments,
>>>>>>>>>>>> Fabian
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
> 


Mime
View raw message