Hi all,
I think we also need to discuss how PRs are reviewed, approved or rejected.
I just read the CONTRIBUTING.md file, and it doesn't say much about it.
What I think is paramount here is: *assume good intentions*. No one is here
to make your life harder, or to introduce malicious code. We are all here
to make Polaris better. That is the Apache Way [1] of doing things: as
individuals contributing to Polaris, we have earned the authority to
contribute; as we do it, it's because we genuinely think that our
contribution is good for the project.
Speaking of the Apache Way, I think it is also important to clarify what an
approval or a request for change means in a PR. ASF projects hosted on
GitHub tend to map the ASF decision-making process [2] to a tacit
convention where a PR approval is a +1; a comment is a +0 if positive, or
-0 otherwise; and a request for changes is a hard -1, effectively blocking
the PR. In this vision, *a -1 should be used sparingly*, and only with good
reasons.
And lastly, let me expand on my opinion on disagreement. As the project
grows, *disagreement will grow as well*. Situations where PRs would end up
in endless discussions may well arrive, or even worse, PRs could end up in
the dustbin of history due to lassitude (hello, Iceberg!), too much
discussion, or lack of consensus.
We want to avoid that, and have things moving forward rather than stall. In
conflictual cases, I think the right mindset is to apply two principles:
1. The "disagree and commit" principle [3]: if consensus cannot be
reached, but the PR is generally approved (no -1s), those who disagree
*must* abide by the decision made by the majority, and move on - no
resentment, no grudges, no hard feelings.
2. As Voltaire said: "the best is the enemy of good" ("le mieux est
l'ennemi du bien"). IOW, I would rather approve an imperfect PR that goes
in the right direction, rather than ask for changes until it becomes
perfect. Code can be amended, and even reverted if need be.
Iceberg has a few guidelines around consensus, review process and
disagreement; we could maybe look into improving ours and include ideas I
outlined above.
Thanks,
Alex
[1]: https://www.apache.org/theapacheway
[2]: https://www.apache.org/foundation/how-it-works/#decision-making
[3]: https://en.wikipedia.org/wiki/Disagree_and_commit
On Fri, Jan 17, 2025 at 11:26 AM Jean-Baptiste Onofré <j...@nanthrax.net>
wrote:
> Hi Robert
>
> Agree for the 2 days period: it doesn't have to be strict, but up to
> the author. The point is really to give chance to the community to
> review.
>
> About the comments, descriptive comments don't have to be resolved.
> However, for "question" comments, they are worth to be considered and
> discussed. At the end of the day, in GitHub, the PR author can always
> click the "resolved" button, so it's not a merge blocker. The reason
> why I think it would make sense to enabled "resolve discussion to
> merge" in .asf.yaml, it's because it can help the PR author without
> missing comments.
>
> I agree with "soft-rule": that's why, instead of guidelines, I would
> rather add "good practices" in the contributor guide.
>
> Regards
> JB
>
> On Fri, Jan 17, 2025 at 8:54 AM Robert Stupp <sn...@snazy.de> wrote:
> >
> > I agree with most points. But I'm not in favor of such strict and
> > potentially enforced rules like "(at least) 2 days". Also not in favor
> > of "all comments must be resolved", because some comments add
> > descriptive value for later - resolving those comments renders those
> > invisible and hard to inspect in the future.
> >
> > As I mentioned in another thread, bugs and especially security issues
> > need to be fixed asap. Same for immediate follow-ups to fix a recently
> > merged PR. Also nits/typos really do not need that enforced rule.
> >
> > This is a very young project. And with project I do not mean only the
> > code base but also the contributors - and with contributors not only
> > people who write code.
> >
> > I think we're all in agreement that the Polaris code base is not in a
> > production ready and mature state yet and have no GA release. A lot of
> > changes need to happen to get there. We should not assume that something
> > is ever set in stone, nor that everything is 100% perfect. We should aim
> > to build great things in a great way. I also think that we have to move
> > fast, the world will not stop turning just because we have to stick to
> > some 2-day rules that we set up.
> >
> > I'd be in favor of a "soft rule": The bigger the topic, the more
> > attention/discussion/information and likely time is required.
> >
> > PS: Friction and follow-up discussions can be a good thing, and
> > sometimes "venting" can be as well.
> >
> >
> > On 17.01.25 07:21, Jean-Baptiste Onofré wrote:
> > > Hi Mike
> > >
> > > Yes agree to fix bug faster than 2 days. I was more thinking about
> > > improvements or new stuff.
> > >
> > > If we have a consensus, I’m happy to create a PR to update the
> contributing
> > > guide.
> > >
> > > Thanks !
> > > Regards
> > > JB
> > >
> > > Le ven. 17 janv. 2025 à 06:20, Michael Collado <collado.m...@gmail.com>
> a
> > > écrit :
> > >
> > >> Hi JB
> > >>
> > >> This is a great topic and worth spending time on. Naturally, we want
> to
> > >> move fast and get things out quickly, but it’s also important that
> everyone
> > >> has a chance to weigh in on changes. This is easy to miss when we’re
> spread
> > >> across so many time zones and some groups are working while others are
> > >> sleeping.
> > >>
> > >> I agree with all points, with the caveat that real actual bug fixes
> should
> > >> be able to be checked in before two days.
> > >>
> > >> I would also propose that interface changes or other substantial
> changes
> > >> should be proposed in an issue and optionally a proposal doc. Some of
> the
> > >> existing interfaces are natural extension points so some users will
> already
> > >> have their own customizations. Interface changes, changes development
> tool
> > >> (e.g., the integration tests structure, the build configuration), and
> the
> > >> application structure all warrant some heads up to users who may have
> a
> > >> stake in these changes.
> > >>
> > >> Personally, I like a lot of the activity I see going on (those black
> box
> > >> tests are 🔥), but I wish I understood some of the decision making
> that was
> > >> going on before a PR is posted and merged.
> > >>
> > >> I think friction for getting changes in needs to be low so that
> people are
> > >> incentivized to make contributions, but we also have to balance that
> with
> > >> ensuring everyone feels they have a chance to have a say.
> > >>
> > >> Maybe we need to codify some of these ideas in the Contributing
> guidelines?
> > >>
> > >> Mike
> > >>
> > >> On Thu, Jan 16, 2025 at 8:56 PM Jean-Baptiste Onofré <j...@nanthrax.net
> >
> > >> wrote:
> > >>
> > >>> Hi folks,
> > >>>
> > >>> It's great to see soooo much activity on Apache Polaris right now !
> > >>>
> > >>> By experience, this high activity can introduce frustration in the
> > >>> community because contributors might be "lost" or have the feeling of
> > >>> missing important changes/proposals.
> > >>>
> > >>> I propose some good practices we could use to improve our
> > >>> collaboration and facilitate team work:
> > >>> 1. GH Issues & PRs should be descriptive and accurate, especially the
> > >>> description should describe a minimum the purpose and the labels
> > >>> should be meaningful (a bug is a breaking change, an improvement is a
> > >>> change on existing, etc).
> > >>> 2. We should avoid duplicating PRs (like creating a new PR and
> closing
> > >>> another one on the same topic), else we lose the history and
> comments.
> > >>> 3. All discussions in a PR should be resolved before merging,
> > >>> 4. I propose to wait 2 working days before merging PR to give a
> chance
> > >>> to contributors to take a look.
> > >>> 5. It's good to take the time for discussion in PR. If you think this
> > >>> discussion needs to move on the dev mailing list (to include the
> whole
> > >>> community) that's also fine.
> > >>>
> > >>> Regarding 3 and 4, I think I can update the .asf.yaml to enforce
> that.
> > >>>
> > >>> Thoughts ?
> > >>>
> > >>> Regards
> > >>> JB
> > >>>
> > --
> > Robert Stupp
> > @snazy
> >
>
