Thanks, I still have similar concerns as my original ones. There is some ambiguity in the first paragraph that might lead to inconsistencies in how different committers handle potential conflicts. Some might be overly cautious even when it’s unnecessary, while others might proceed with reviews even in situations where there is a clear conflict, but they don't recognize it as such.
Also, I would avoid basing what is worth discussion on the number of files. It is better to define criteria around public APIs/spec changes/REST APIs, etc (I see some of those in the current doc, +1 to that). Would be great to have a discussion around the specific criteria for changes requiring a discussion, vote, etc from API/spec perspective. Also, it is not very clear when to use IIP vs voting vs discussion. I think the last two paragraphs can benefit from more clarity on the criteria. For example, if the committer were to follow a flowchart to test whether a PR is mergeable, what would the flowchart look like? (Of course, we do not have to depict a flowchart, but I am just conveying the style of guidance that could potentially be more effective and clear). Thanks, Walaa. On Fri, Aug 9, 2024 at 8:05 AM Micah Kornfield <emkornfi...@gmail.com> wrote: > As a summary, I think we are potentially at consensus. I think there are > some concerns but not hard blockers: > > 1. Having a vote for every spec change might be too onerous (we can maybe > change this policy once we have a sense of the overhead). > 2. Some of the content is slightly repetitive with other documentation in > ASF (I've tried to shorten this to minimal possible length with links). > 3. Rules can be used to point fingers instead of having constructive > conversations on how to improve process or use judgement. > > Please comment on the PR or reply here if we need more discussion. If > there aren't other items I'll start a vote next week to merge the change. > > Thanks, > Micah > > On Tue, Aug 6, 2024 at 12:18 PM Micah Kornfield <emkornfi...@gmail.com> > wrote: > >> My only question is around "conflict of interest" while reviewing PRs. I >>> think it needs further explanation and some concrete examples. >> >> >> I was hesitant to add details here as I thought people could use >> reasonable judgement and to my knowledge I don't think the project has had >> to deal with PR that represented a conflict. Since this has come up a few >> times I added the following text: "There is no strict definition of >> conflict of interest. A conflict of interest is most likely to occur when >> the author of a pull request and the reviewer share an employer, and the >> change in question could be construed as providing preferential treatment >> to their shared employer. " >> >> Hopefully, this is sufficient for a first draft and if there is a >> necessity for more clarity the wording can be refined and more concrete >> examples can be given if this starts to be a problem in the community >> (happy to also refine this language on the PR if others want to reword or >> add examples that I'm not aware of). >> >> Thans, >> Micah >> >> On Tue, Aug 6, 2024 at 11:55 AM Anton Okolnychyi <aokolnyc...@gmail.com> >> wrote: >> >>> The doc in its current form seems reasonable to me. I understand it >>> reiterates some of the ASF guidelines but I don't mind that given that it >>> is fairly concise. My only question is around "conflict of interest" while >>> reviewing PRs. I think it needs further explanation and some concrete >>> examples. >>> >>> - Anton >>> >>> нд, 4 серп. 2024 р. о 15:04 Micah Kornfield <emkornfi...@gmail.com> >>> пише: >>> >>>> Thank you for the feedback. I've put some replies inline to specific >>>> points below. >>>> >>>> >>>>> "Request Changes" should only be used to literally block a PR from >>>>> being >>>>> merged - either for timing issues (e.g. "can only be merged right after >>>>> a release is out, because it changes public docs") or because of really >>>>> fundamental issues with the change, with a clear and precise >>>>> justification. >>>> >>>> >>>> I'm not sure I'm understanding this correctly, but it sounds like a >>>> fundamentally different usage of how "request changes" is used in my >>>> experience (which is typically, reviewer is done reviewing and there are >>>> still outstanding issues to address). IIUC this actually is a different >>>> usage as what is currently described in the PR? For the cases mentioned >>>> here, especially the latter it seems like this should be specifically >>>> mentioned on the PR (and it's not clear Request changes by itself is >>>> sufficient) . >>>> >>>> "Large changes" (5 lines? 5000 lines?) is also rather subjective - >>>>> proposal: "User facing functional and behavior changes". >>>> >>>> >>>> I changed the Iceberg improvement proposal language to be functional >>>> and behavior changes to the specification. I also added in a bullet point >>>> which says changes affecting more than 20 files must be discussed on the >>>> mailing list (20 was chosen arbitrarily but seems to be higher than any >>>> file count observed in sampling ~30 PRs that are currently open in the >>>> repo). >>>> >>>> [1] A few paragraphs/statements that delegate to the comitter's >>>>> judgement call. (e.g., "Committer is trusted", "If committer feels" >>>>> ..). So the value of adding them is not very clear to me. >>>> >>>> >>>> [2] A few things that are implied (e.g., re-iterating ASF guidelines, >>>>> lack of agreement will definitely lead to not merging the PR -- >>>>> whether to discuss the topic as a proposal or not should have a >>>>> separate process to state what qualifies as a proposal discussion). >>>> >>>> I would avoid [1] and [2] to avoid repetition or too much reading of >>>>> those rules, especially when they are subjective (e.g., [1]) or >>>>> implied (e.g., [2]). >>>> >>>> >>>> I've tried to remove as much language here as possible and provide >>>> links to Apache documents. The value it provides is documenting norms so >>>> new contributors and committers understand the responsibility of the roles >>>> for doing day to day work. I imagine it will be much more common for people >>>> to read the contributing guide first and then possibly any ASF >>>> documentation. Leaving things as implied by ASF guidelines works for >>>> people already familiar with them, but not for newcomers (and the ASF >>>> documentation is considerably more verbose than the current text). Also, >>>> some things are not defined in ASF guidelines (I tried to clarify that code >>>> changes in Iceberg follow Review then Commit, since they require a second >>>> reviewer). >>>> >>>> Last, despite ASF guidelines there have been some people expressing the >>>> opinion on the mailing list that some reviews were blocked on specific >>>> people having to review them, I think a reminder that this shouldn't be the >>>> case is useful (or if there are specific people that need to review some >>>> changes we can add guidelines as necessary). >>>> >>>> What remains (e.g., [3] or the way to proceed if >>>>> a committer feels something is worthy of a proposal-level discussion) >>>>> fits more in a process that organizes what qualifies as a proposal vs >>>>> code change etc (e.g., the rule should not be about "whether a >>>>> committer feels that something is worth a proposal", but rather there >>>>> should be a process that guides both the contributor and committer for >>>>> what qualifies as proposal vs a direct PR). >>>> >>>> >>>> If I am understanding correctly, this is saying more guidelines for >>>> what should go through the improvement proposals should be established. >>>> This sounds reasonable to me, but it also seems like it can be handled >>>> independently in a follow-up discussion/pull request? >>>> >>>> Thanks, >>>> Micah >>>> >>>> >>>> >>>> >>>> >>>> On Sat, Aug 3, 2024 at 1:14 AM Robert Stupp <sn...@snazy.de> wrote: >>>> >>>>> I agree with Walaa's comments and appreciate Micah's intent to clarify >>>>> things! >>>>> >>>>> Sticking to a few important rules, using a crisp phrasing, and leaving >>>>> out the subjective, optional and "ASF implied" things, would be better. >>>>> >>>>> "Large changes" (5 lines? 5000 lines?) is also rather subjective - >>>>> proposal: "User facing functional and behavior changes". >>>>> >>>>> I'm +1 on clarifying what "Request Changes" on a PR means. Some >>>>> cultures >>>>> can interpret this as a "push back", so I'd like to propose that >>>>> "Request Changes" should only be used to literally block a PR from >>>>> being >>>>> merged - either for timing issues (e.g. "can only be merged right >>>>> after >>>>> a release is out, because it changes public docs") or because of >>>>> really >>>>> fundamental issues with the change, with a clear and precise >>>>> justification. >>>>> >>>>> Robert >>>>> >>>>> On 02.08.24 21:06, Walaa Eldin Moustafa wrote: >>>>> > My concern with this change (in its current form) is that it combines >>>>> > (mixes?) three things: >>>>> > >>>>> > [1] A few paragraphs/statements that delegate to the comitter's >>>>> > judgement call. (e.g., "Committer is trusted", "If committer feels" >>>>> > ..). So the value of adding them is not very clear to me. >>>>> > [2] A few things that are implied (e.g., re-iterating ASF guidelines, >>>>> > lack of agreement will definitely lead to not merging the PR -- >>>>> > whether to discuss the topic as a proposal or not should have a >>>>> > separate process to state what qualifies as a proposal discussion). >>>>> > [3] A few crisp rules, e.g, how to treat changes under spec and files >>>>> > under the `format` directory. >>>>> > >>>>> > I would avoid [1] and [2] to avoid repetition or too much reading of >>>>> > those rules, especially when they are subjective (e.g., [1]) or >>>>> > implied (e.g., [2]). What remains (e.g., [3] or the way to proceed if >>>>> > a committer feels something is worthy of a proposal-level discussion) >>>>> > fits more in a process that organizes what qualifies as a proposal vs >>>>> > code change etc (e.g., the rule should not be about "whether a >>>>> > committer feels that something is worth a proposal", but rather there >>>>> > should be a process that guides both the contributor and committer >>>>> for >>>>> > what qualifies as proposal vs a direct PR). >>>>> > >>>>> > Thanks, >>>>> > Walaa. >>>>> > >>>>> > >>>>> > On Fri, Aug 2, 2024 at 11:43 AM Micah Kornfield < >>>>> emkornfi...@gmail.com> wrote: >>>>> >> I just wanted to follow up on this. >>>>> >> >>>>> >> Ryan and Szehon unless you strongly object to the current >>>>> formulation of the PR, I'd like to move forward with a vote to merge. >>>>> There are already a fair number of people in the community that have >>>>> reviewed and seemed to think the proposal is not unreasonable. For votes >>>>> it seems like most of the recent threads on the matter have decided to err >>>>> on the side of votes for changes to specs, as it reduces the judgement >>>>> call >>>>> of reviewers. Thoughts? >>>>> >> >>>>> >> Thanks, >>>>> >> Micah >>>>> >> >>>>> >> On Tue, Jul 30, 2024 at 11:08 AM Micah Kornfield < >>>>> emkornfi...@gmail.com> wrote: >>>>> >>>> The problem I'm worried about is the tendency to misuse docs like >>>>> this and become focused on it as a rule. People tend to apply written >>>>> rules >>>>> mechanically and I worry about people substituting a reading of this text >>>>> for judgement. For example, a strict reading of "encouraged to ask another >>>>> committer" means that it is optional. >>>>> >>> >>>>> >>> I agree with this concern which is why I tried to make everything >>>>> optional. I hope if there ever needs to be a revision here it is framed >>>>> as >>>>> constructive and not a blame game on someone breaking the rules. In >>>>> general, I think placing the reminder on trust and putting the project >>>>> first are good reminders in either case. IMO the text is short enough that >>>>> they should be included directly in the contributors guideline. >>>>> >>> >>>>> >>> >>>>> >>>> To me, (2) is a bit new here and in the grey area for >>>>> interpretation. I was thinking about this while reviewing >>>>> https://github.com/apache/iceberg/pull/10793, which could be a >>>>> category (2) and non-functional change but would need a full >>>>> code-modification vote as per [Iceberg improvement >>>>> proposal](#apache-iceberg-improvement-proposals). I can see both sides, >>>>> to >>>>> avoid a potential dispute/misunderstanding over the clarification, it >>>>> would >>>>> be nice to have a vote on the devlist. But it may also be yet another >>>>> burden, when something can be more easily decided on the github discussion >>>>> itself via approval by the relevant parties. >>>>> >>> >>>>> >>> The PR documents how I understood the requirement communicated to >>>>> me about spec modifications. I don't recall seeing exactly where this >>>>> requirement was agreed. so maybe we can revise it. IMO spec changes are >>>>> important enough that it is worth explicit email to the mailing list >>>>> notifying people of the proposed change. A vote might be too heavyweight >>>>> though. Would updating the guideline to require a separate email to the >>>>> mailing list for changes and requiring the change be open for at least 72 >>>>> hours strike the right balance? >>>>> >>> >>>>> >>> Thanks, >>>>> >>> Micah >>>>> >>> >>>>> >>> >>>>> >>> >>>>> >>> >>>>> >>> >>>>> >>> >>>>> >>> On Mon, Jul 29, 2024 at 3:08 PM Szehon Ho <szehon.apa...@gmail.com> >>>>> wrote: >>>>> >>>> Typo , wrong link: >>>>> >>>> (2) requiring full code-modification vote as per [Iceberg >>>>> improvement proposal](#apache-iceberg-improvement-proposals) => full >>>>> code-modification vote as per [code modification]( >>>>> https://www.apache.org/foundation/voting.html#votes-on-code-modification >>>>> ) >>>>> >>>> >>>>> >>>> On Mon, Jul 29, 2024 at 1:53 PM Szehon Ho < >>>>> szehon.apa...@gmail.com> wrote: >>>>> >>>>> Hi, >>>>> >>>>> >>>>> >>>>> Also if I read it correctly, I think this proposal imposes the >>>>> following workflows in "spec" folders : >>>>> >>>>> >>>>> >>>>> Large and functional changes. These redirect to Iceberg >>>>> improvement proposals, which ends in code-modification vote >>>>> >>>>> bug-fixes or clarification, which is specified to require >>>>> code-modification vote >>>>> >>>>> grammar, spelling, minor formatting fix, not covered here (I >>>>> guess it is like any normal code review?) >>>>> >>>>> >>>>> >>>>> To me, (2) is a bit new here and in the grey area for >>>>> interpretation. I was thinking about this while reviewing >>>>> https://github.com/apache/iceberg/pull/10793, which could be a >>>>> category (2) and non-functional change but would need a full >>>>> code-modification vote as per [Iceberg improvement >>>>> proposal](#apache-iceberg-improvement-proposals). I can see both sides, >>>>> to >>>>> avoid a potential dispute/misunderstanding over the clarification, it >>>>> would >>>>> be nice to have a vote on the devlist. But it may also be yet another >>>>> burden, when something can be more easily decided on the github discussion >>>>> itself via approval by the relevant parties. So I think I would agree >>>>> with >>>>> Ryan in mentioning that significant (would maybe add "functional") spec >>>>> change needs a vote on the dev list. >>>>> >>>>> >>>>> >>>>> Thanks >>>>> >>>>> Szehon >>>>> >>>>> >>>>> >>>>> On Mon, Jul 29, 2024 at 1:16 PM Ryan Blue >>>>> <b...@databricks.com.invalid> wrote: >>>>> >>>>>> I think the proposed doc looks good, but I'm not sure that it >>>>> is better to add this to our guidelines. >>>>> >>>>>> >>>>> >>>>>> On one hand the doc describes how ASF communities work in >>>>> general: committers review and commit PRs and are expected to use good >>>>> judgement, ask one another for help when necessary, and broaden the set of >>>>> people in the discussion when there's a disagreement. I really appreciate >>>>> that Micah called out that this is intentionally vague to emphasize >>>>> committer judgement. >>>>> >>>>>> >>>>> >>>>>> The problem I'm worried about is the tendency to misuse docs >>>>> like this and become focused on it as a rule. People tend to apply written >>>>> rules mechanically and I worry about people substituting a reading of this >>>>> text for judgement. For example, a strict reading of "encouraged to ask >>>>> another committer" means that it is optional. >>>>> >>>>>> >>>>> >>>>>> Given that the majority of the content here is stating how ASF >>>>> communities work and the only Iceberg-specific parts are the proposal >>>>> process and calling out that we vote on spec changes, I would probably >>>>> just >>>>> have a description of how to handle proposals (which is already there) and >>>>> a note that significant spec changes should use a vote on the dev list. >>>>> >>>>>> >>>>> >>>>>> On Sun, Jul 28, 2024 at 11:15 PM Jean-Baptiste Onofré < >>>>> j...@nanthrax.net> wrote: >>>>> >>>>>>> Hi Micah >>>>> >>>>>>> >>>>> >>>>>>> Thanks ! It looks good to me now you have included comments >>>>> from everyone. >>>>> >>>>>>> >>>>> >>>>>>> Regards >>>>> >>>>>>> JB >>>>> >>>>>>> >>>>> >>>>>>> On Fri, Jul 26, 2024 at 1:15 AM Micah Kornfield < >>>>> emkornfi...@gmail.com> wrote: >>>>> >>>>>>>> As part of the bylaws discussions that have been happening, >>>>> we are trying to make small focused proposals to move things forward. As >>>>> a >>>>> first step towards this I created a proposal for guidelines on committing >>>>> pull requests [1]. Feedback is appreciated. >>>>> >>>>>>>> >>>>> >>>>>>>> Given the level of interest in the discussions so far, it >>>>> seems that the best path forward is to hold an official vote before >>>>> merging. I intend to do this once we appear to have consensus but if the >>>>> people prefer we can try to avoid the overhead. >>>>> >>>>>>>> >>>>> >>>>>>>> Thanks, >>>>> >>>>>>>> Micah >>>>> >>>>>>>> >>>>> >>>>>>>> >>>>> >>>>>>>> [1] https://github.com/apache/iceberg/pull/10780 >>>>> >>>>>> >>>>> >>>>>> >>>>> >>>>>> -- >>>>> >>>>>> Ryan Blue >>>>> >>>>>> Databricks >>>>> >>>>> -- >>>>> Robert Stupp >>>>> @snazy >>>>> >>>>>
