Hi, Let's rename from "guidelines" to "good practices & advices" :)
The purpose here is not to have strict enforcement, it's more to assist contributors about good practices to have a better collaboration. There's no problem to "deviate" from these good practices with good reasons and transparency. The idea is more to "frame" what we discuss. In this regard: - I propose to create a PR to update the contributing guide with this good practices discussed in this thread - I propose to close https://github.com/apache/polaris/pull/840 in favor of "soft rule" (we quickly discuss about owner review with CODEOWNERS in this PR, but let's keep aside for now) - I propose to update https://github.com/apache/polaris/pull/839. We have a consensus about the label, but no consensus about the schedule for now. In order to move forward, I propose to update the PR with just the label for now (as we have a consensus) and see if it's enough. If it's still a concern, I will create a new PR to update the schedule Thoughts ? Thanks ! Regards JB On Thu, Jan 23, 2025 at 5:39 AM Michael Collado <collado.m...@gmail.com> wrote: > > I agree with Russell 100%. I think the intent here is to codify a spirit of > collaboration. That’s always going to be imperfect. But clarifying > expectations is important, even if it’s imperfect. > > For some of the requirements (e.g., waiting an appropriate period or > waiting merging with unresolved comments), I think there are ways to set up > merge gates that can be bypassed with a label. I know we have something > similar setup at Snowflake. E.g., if someone can’t wait two days, they > could add a label that allows bypassing the merge gates. I can look into > how that works, if you like. > > Regarding the "stable APIs", personally I think we should bias toward open > rather than bias toward closed. If an interface is marked public and in a > package that's not specifically marked for internal use, it should be > assumed to be open. Biasing closed means that we have to explicitly > annotate APIs that are intended to be publicly accessible - people browsing > the code may not know that that's the convention we've gone with. I know I > certainly don't read the contributing guide for every library I add a > compile-time dependency on. Biasing open means we have to explicitly > annotate APIs that are expected to be internal only. Those annotations and > the javadoc comments make it clear for any user who's trying to add a > compile-time dependency on something, making it glaringly obvious that they > should not do so (or at least, that doing so they are explicitly taking the > risk that something will change without warning). I think the bias should > also follow similarly for stable v experimental APIs. Anything not > explicitly annotated as @Experimental is considered stable. > > More than java interfaces, we have to be cognizant of everything > user-facing - this includes the configuration files, health checker > endpoints, metric names... there are people who currently have Polaris > deployed to production and some of them have alarms and dashboards that > matter to them. > > I acknowledge that this is a tough balancing act. Iteration speed matters > and it's important that we have the ability to make mistakes and go back > and fix them - otherwise we end up in analysis paralysis because nobody > wants to merge the imperfect API that has to last forever. It should be ok > to merge mistakes and to go back and fix them, but I think we can do that > and still have good general guidelines that move everybody toward more > communication and more collaboration, even if those guidelines aren't > perfect. > > Mike > > > > On Wed, Jan 22, 2025 at 11:15 AM Russell Spitzer <russell.spit...@gmail.com> > wrote: > > > The proposal sounds good to me, I really don't think the details matter > > that much here. > > > > If someone is arguing the wording of the guidelines ("I waited two > > mercurial days, it didn't specify in the guidelines.") > > then something has gone wrong just as much as having broken them. Even if > > we specified the > > guidelines so well that no one could argue against their wording, we > > wouldn't use the > > guidelines as the justification for an action in the repository. If you > > attempt to follow the guidelines > > and make a mistake, or even follow them accurately but find out that by > > no-ones fault someone > > who should have been notified wasn't able to be you should work to find a > > common > > ground that doesn't just involve "too bad, I followed the rules." > > > > There will be exceptions to these guidelines as well I'm sure, we just need > > to be clear that if you are > > violating these principles you have a very good reason for doing so. > > > > For example for an immediate security purpose, just make sure you can > > clearly articulate why that > > merge needed to happen immediately and be prepared to defend it to the > > community. > > > > Need to open a duplicate of a pr that was already closed? I'm sure there > > are situations in which you > > might want to do so. You can do so, but you better have a reason that isn't > > "I wanted a different outcome". > > Did circumstances change? Has the project evolved in a different direction? > > Someone left the project? > > All of these could be valid reasons and I don't think we gain anything by > > articulating everything. > > > > The key principle here is to treat other contributors as collaborators and > > I fully approve of that. I don't feel > > strongly about encoding the rules in github itself. > > > > > > On Wed, Jan 22, 2025 at 12:05 PM Dmitri Bourlatchkov <di...@apache.org> > > wrote: > > > > > On Wed, Jan 22, 2025 at 12:58 PM Jean-Baptiste Onofré <j...@nanthrax.net> > > > wrote: > > > > > > > It's not a problem to have several people working on the same feature. > > > > > > > > By duplicate PRs, I mean that you open a PR, where you get comments, > > > > changes requested, etc. Then, you close this PR to open a new one (on > > > > the same topic). So, I mean by a single person. I'm more in favor of > > > > refactoring an existing PR, instead of closing PR #1 to open PR #2 > > > > with the refactoring. > > > > On some projects, I saw contributors closing the PR #1, creating PR #2 > > > > just to get approval and merge (loosing the discussion on #1). > > > > > > > > > > Thanks for the clarification! I totally agree with this. > > > > > > Cheers, > > > Dmitri. > > > > >
