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 >> >>
