Hi Alex All wise words, thanks :)
I think we are all on the same page about good intentions. About PR approval, I agree it's close to the ASFvoting process (even though not exactly the same as fragment votes are allowed for code modification vote). If needed, we can always bring discussion/review on the dev mailing list (see https://www.apache.org/foundation/voting.html about code modification). I also agree about "fast good enough merge" and if needed revisit later (it's what I proposed in my previous message: merge and if needed it could be reviewed). Before git/github, at the ASF we used the commit and review paradigm (instead of review and commit), and it works for sure :). It's always better to move forward on a good enough PR rather than chasing the best PR. I'm not sure we need anything more that the resources we already have at foundation level: - https://community.apache.org/committers/decisionMaking.html - https://www.apache.org/foundation/voting.html My proposal is more to write down obvious good practices in the contributing guide, also referring to the foundation resources. One key point was: we have to be sure to give a chance for the community to review a change. We are all doing great work ! Thanks everyone and let's move forward: we have a bunch of cool stuff coming in Polaris :) Regards JB On Fri, Jan 17, 2025 at 2:27 PM Alex Dutra <alex.du...@dremio.com> wrote: > > 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: > > 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. > 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 >> >
