I think we can't do that since we don't have direct control over the
github repository, similar to how we have to rely on the bot to close PR's.
On 17.10.2015 12:32, Alexander Alexandrov wrote:
One suggestion from me: in GitHub you can make clear who the current
sheppard is through the "Assignee" field in the PR (which can and IMHO
should be different from the user who actually opened the request).
Regards,
A.
2015-10-16 15:58 GMT+02:00 Fabian Hueske <fhue...@gmail.com>:
Hi folks,
I think we can split of the discussion of a PR meeting.
Are there any more comments on the proposal itself?
Otherwise, I would go ahead and put the described process (modulo the
comments) into a document for our website.
Cheers, Fabian
2015-10-07 16:57 GMT+02:00 Fabian Hueske <fhue...@gmail.com>:
I like the idea of meeting once a week to discuss about PRs as well.
Regarding lingering PRs, you can simply sort the Flink PRs in Github by
"least recently updated"
Cheers,
Fabian
2015-10-07 16:48 GMT+02:00 Theodore Vasiloudis <
theodoros.vasilou...@gmail.com>:
Could we maybe do a "PR overall status assessment" once per week or
so,
where we find those problematic PRs and try to assign them / close
them?
I like this idea, as it would raise awareness about lingering PRs. Does
anybody know if there is
some way to integrate this into JIRA, so we can easily see (and
filter/sort) lingering PRs?
Cheers,
Theo
On Wed, Oct 7, 2015 at 12:04 PM, Vasiliki Kalavri <
vasilikikala...@gmail.com
wrote:
Hey,
I agree that we need to organize the PR process. A PR management tool
would
be great.
However, it seems to me that the shepherding process described is
-more
or
less- what we've already been doing. There is usually a person that
reviews
the PR and kind-of drives the process. Maybe making this explicit will
make
things go faster.
There is a problem I see here, quite related to what Theo also
mentioned.
For how long do we let a PR lingering around without a shepherd? What
do we
do if nobody volunteers?
Could we maybe do a "PR overall status assessment" once per week or
so,
where we find those problematic PRs and try to assign them / close
them?
Finally, I think shepherding one's own PR is a bad idea (the review
part)
unless it's something trivial.
Cheers and see you very soon,
-Vasia.
On Oct 7, 2015 11:24 AM, "Matthias J. Sax" <mj...@apache.org> wrote:
Ok. That makes sense. So most people will need to change behavior
and
start discussions in JIRA and not over dev list. Furthermore, issues
list must be monitored more carefully... (I personally, watch dev
carefully and only skip over issues list right now)
-Matthias
On 10/07/2015 10:22 AM, Fabian Hueske wrote:
@Matthias: That's a good point. Each PR should be backed by a JIRA
issue.
If that's not the case, we have to make the contributor aware of
that
rule.
I'll update the process to keep all discussions in JIRA (will be
mirrored
to issues ML), OK?
@Theo: You are right. Adding this process won't be the silver
bullet to
fix
all PR related issues.
But I hope it will help to improve the overall situation.
Are there any other comment? Otherwise I would start to prepare
add
this
to
our website.
Thanks, Fabian
2015-10-06 9:46 GMT+02:00 Theodore Vasiloudis <
theodoros.vasilou...@gmail.com>:
One problem that we are seeing with FlinkML PRs is that there are
simply
not enough commiters to "shepherd" all of them.
While I think this process would help generally, I don't think it
would
solve this kind of problem.
Regards,
Theodore
On Mon, Oct 5, 2015 at 3:28 PM, Matthias J. Sax <
mj...@apache.org>
wrote:
One comment:
We should ensure that contributors follow discussions on the dev
mailing
list. Otherwise, they might miss important discussions regarding
their
PR (what happened already). Thus, the contributor was waiting
for
feedback on the PR, while the reviewer(s) waited for the PR to
be
updated according to the discussion consensus, resulting in
unnecessary
delays.
-Matthias
On 10/05/2015 02:18 PM, Fabian Hueske wrote:
Hi everybody,
Along with our efforts to improve the “How to contribute”
guide,
I
would
like to start a discussion about a setting up a review process
for
pull
requests.
Right now, I feel that our PR review efforts are often a bit
unorganized.
This leads to situation such as:
- PRs which are lingering around without review or feedback
- PRs which got a +1 for merging but which did not get merged
- PRs which have been rejected after a long time
- PRs which became irrelevant because some component was
rewritten
- PRs which are lingering around and have been abandoned by
their
contributors
To address these issues I propose to define a pull request
review
process
as follows:
1. [Get a Shepherd] Each pull request is taken care of by a
shepherd.
Shepherds are committers that voluntarily sign up and *feel
responsible*
for helping the PR through the process until it is merged (or
discarded).
The shepherd is also the person to contact for the author of
the
PR. A
committer becomes the shepherd of a PR by dropping a comment on
Github
like
“I would like to shepherd this PR”. A PR can be reassigned with
lazy
consensus.
2. [Accept Decision] The first decision for a PR should be
whether
it
is
accepted or not. This depends on a) whether it is a desired
feature
or
improvement for Flink and b) whether the higher-level solution
design
is
appropriate. In many cases such as bug fixes or discussed
features
or
improvements, this should be an easy decision. In case of more
a
complex
feature, the discussion should have been started when the
mandatory
JIRA
was created. If it is still not clear whether the PR should be
accepted
or
not, a discussion should be started in JIRA (a JIRA issue needs
to
be
created if none exists). The acceptance decision should be
recorded
by
a
“+1 to accept” message in Github. If the PR is not accepted, it
should
be
closed in a timely manner.
3. [Merge Decision] Once a PR has been “accepted”, it should be
brought
into a mergeable state. This means the community should quickly
react
on
contributor questions or PR updates. Everybody is encouraged to
review
pull
requests and give feedback. Ideally, the PR author does not
have
to
wait
for a long time to get feedback. The shepherd of a PR should
feel
responsible to drive this process forward. Once the PR is
considered
to
be
mergeable, this should be recorded by a “+1 to merge” message
in
Github.
If
the pull request becomes abandoned at some point in time, it
should
be
either taken over by somebody else or be closed after a
reasonable
time.
Again, this can be done by anybody, but the shepherd should
feel
responsible to resolve the issue.
4. Once, the PR is in a mergeable state, it should be merged.
This
can
be
done by anybody, however the shepherd should make sure that it
happens
in a
timely manner.
IMPORTANT: Everybody is encouraged to discuss pull requests,
give
feedback,
and merge pull requests which are in a good shape. However, the
shepherd
should feel responsible to drive a PR forward if nothing
happens.
By defining a review process for pull requests, I hope we can
- Improve the satisfaction of and interaction with contributors
- Improve and speed-up the review process of pull requests.
- Close irrelevant and stale PRs more timely
- Reduce the effort for code reviewing
The review process can be supported by some tooling:
- A QA bot that checks the quality of pull requests such as
increase
of
compiler warnings, code style, API changes, etc.
- A PR management tool such as Sparks PR dashboard (see:
https://spark-prs.appspot.com/)
I would like to start discussions about using such tools later
as
separate
discussions.
Looking forward to your feedback,
Fabian
