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
