Initial committer guidelines and requirements for merging #10780

emkornfield · 2024-07-25T06:32:29Z

Based on mailing list discussion add focused language to contributing.md on committing PRs.

adutra · 2024-07-25T07:52:52Z

site/docs/contribute.md

+There are several exceptions to this process:
+
+* Large changes or functional changes to a specification must go through the an [Iceberg improvement proposal](#apache-iceberg-improvement-proposals) before any code can be merged.
+* Changes to files under the `format` directory and `open-api/rest-catalog*` are considered specification changes. Unless already covered under an Iceberg improvement proposal, specification changes require their own vote (e.g. bug fixes or specification clarifications). The vote follows the ASF [code modification][apache-vote] model with three positive PMC votes required and no lazy consensus modifier. Grammar, spelling and minor formatting fixes are exempted from this rule.


We also have aws/src/main/resources/s3-signer-open-api.yaml; would changes to that file fall under the "specification changes" category?

I don't know. Seems AWS specific based on the directory structure? @jackye1995 ?

yes, but that is not contributed by AWS. It was added to support remote signing feature in Tabular. I think it is more like a vendor integration feature, I would not consider that a part of the official REST spec. Thoughts? @nastra @danielcweeks

Fokko

This makes sense to me, thanks for this writeup @emkornfield

site/docs/contribute.md

Co-authored-by: Fokko Driesprong <fokko@apache.org>

site/docs/contribute.md

Co-authored-by: Fokko Driesprong <fokko@apache.org>

site/docs/contribute.md

Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>

jbonofre · 2024-07-25T22:04:43Z

It looks good to me. Thanks !

By the way, it would be also possible to update .asf.yml for "force" at least one review. I proposed to use labels (using modules/paths), assign reviewers per labels, force at least one approved review per PR.

We could update .asf.yml this way:

  ...
  protected_branches:
    main:
      required_linear_history: true
      required_status_checks:
        strict: true
      required_pull_request_reviews:
        required_approving_review_count: 1
        dismiss_stale_reviews: true
      required_conversation_resolution: true
  ...

Don't get me wrong: I don't propose to update .asf.yml in this PR. It's just a proposal I can bring to the team if it makes sense to you.

emkornfield · 2024-07-25T23:07:59Z

It looks good to me. Thanks !

By the way, it would be also possible to update .asf.yml for "force" at least one review. I proposed to use labels (using modules/paths), assign reviewers per labels, force at least one approved review per PR.

We could update .asf.yml this way:
  ...
  protected_branches:
    main:
      required_linear_history: true
      required_status_checks:
        strict: true
      required_pull_request_reviews:
        required_approving_review_count: 1
        dismiss_stale_reviews: true
      required_conversation_resolution: true
  ...
Don't get me wrong: I don't propose to update .asf.yml in this PR. It's just a proposal I can bring to the team if it makes sense to you.

I'm OK either way. A mechanism doesn't hurt here, but I think in terms of merging I think it is sufficient to operate on a model of trust (I think merging accidentally is pretty hard to do and pretty easy to reverse).

jackye1995 · 2024-07-25T23:23:53Z

site/docs/contribute.md

+Most pull requests can be merged once a committer is satisfied with the code in the PR. For larger changes or additions to public
+APIs committers will wait at least 24 hours before merging to ensure there is no additional feedback from members of the community. As in
+all ASF governed projects committers are expected to act in the best interest of the project. If a committer feels there might be a conflict
+of interest with a pull request they review, they are encouraged to ask for another committer to look at the pull request.


Could we define what "a conflict of interest" means, or at least give any examples?

Does this mean cases like a committer working mainly on pyiceberg go to merge something directly in iceberg-spark? Just to given an arbitrary example.

I left this intentionally vague. In general committers are expected to use their best judgement. If there are explicit examples that have happened in the community where it was believe something should not been committed we can include those. Otherwise, I think this is place that can be refined if the PMC thinks some changes are being merged prematurely.

This goes to the ethos of community over code and trust in committers.

site/docs/contribute.md

stevenzwu · 2024-07-26T00:11:27Z

By the way, it would be also possible to update .asf.yml for "force" at least one review. I proposed to use labels (using modules/paths), assign reviewers per labels, force at least one approved review per PR.

I thought that is already the case. Here is the screenshot from PR #10777 that I submitted. Without any approval, merge button is disabled

site/docs/contribute.md

rdblue · 2024-07-29T18:02:43Z

site/docs/contribute.md

+There are several exceptions to this process:
+
+* Large changes and functional changes to a specification must go through the [Iceberg improvement proposal](#apache-iceberg-improvement-proposals) before any code can be merged.
+* Changes to files under the `format` directory and `open-api/rest-catalog*` are considered specification changes. Unless already covered under an Iceberg improvement proposal, specification changes require their own vote (e.g. bug fixes or specification clarifications). The vote follows the ASF [code modification](https://www.apache.org/foundation/voting.html#votes-on-code-modification) model and no lazy consensus modifier. Grammar, spelling and minor formatting fixes are exempted from this rule.


We may want to allow for draft specifications, so we can publish changes as we develop the reference implementation.

Yes, I agree, I think we probably need more guidance under the Improvement proposals section and this point should be discussed as part of the refinement there?

I guess it doesn't hurt to add something now, I added some language on what I think is reasonable.

How does draft specification work? Could you describe more details?

i put down what I was thinking. Not sure if matches with what @rdblue means?

…udgement

szehon-ho · 2024-07-29T20:10:20Z

site/docs/contribute.md

+There are several exceptions to this process:
+
+* Large changes and functional changes to a specification must go through the [Iceberg improvement proposal](#apache-iceberg-improvement-proposals) before any code can be merged.
+* Changes to files under the `format` directory and `open-api/rest-catalog*` are considered specification changes. Unless already covered under an Iceberg improvement proposal, specification changes require their own vote (e.g. bug fixes or specification clarifications). The vote follows the ASF [code modification](https://www.apache.org/foundation/voting.html#votes-on-code-modification) model and no lazy consensus modifier. Grammar, spelling and minor formatting fixes are exempted from this rule.


Confirming my understanding here:

Functional spec changes go through improvement-process , which in the end require a code modification vote (3 +1 votes without lazy consensus modifier)

Any spec clarification still requires a code modification vote (3 +1 votes without lazy consensus modifier). A current example we are working is #10793.

Grammar, spelling, and minor formatting still go through normal code review process.

This is my understanding (I was trying to codify rules that have come across the mailing list recently).

site/docs/contribute.md

* clarify committer should not be author. Remove content in favor of links. * Separate and define large changes should be discussed on the mailing list.

site/docs/contribute.md

wmoustafa · 2024-08-17T00:54:05Z

site/docs/contribute.md

+
+Requesting changes on a PR indicates a reviewer believes the PR has merit but still needs issues addressed before merging. If a reviewer believes the change should not be merged at all and there is nothing the author could do to address the reviewers concerns, the reviewer should explicitly state this on the PR. In the rare event that a PR author and reviewers cannot come to a consensus on a PR, the disagreement should be raised to the developer mailing list for further discussion. In this context, a reviewer is anyone leaving comments on the PR including contributors, committers and PMC members.
+
+There are several exceptions to a single committer being able to merge a PR:


Is there a way to reframe this differently? It sounds like the below and single committer merging a PR are not mutually exclusive, because after the below happens, it is quite possible that a single committer reviews and merges, right?

Something about seeking feedback from the community?

My intention was more on the structure. Basically, I think the intended message here is there are changes that can be merged through the regular PR process solely, and other changes require following more formal process, like dev list discussion, IIP, vote etc. I think something like this could make it clearer:

There are two types of changes: those that can be merged through the regular PR process, and those that require a more formal procedure in addition to the PR process, such as a developer mailing list discussion, an Iceberg Improvement Proposal (IIP), or a vote.
Then we can describe (1) the regular process and (2) the more formal process required before the regular process.

It is essentially the same content but with a clearer message about the relationship between the processes (e.g., the latter is a pre-requisite of the former in some cases).

I added a parenthetical "(exceptions that require additional input from the community are detailed below)" in the first paragraph. which I think address this concern. @wmoustafa please let me know if that captures the intent of your suggestion.

Thanks @emkornfield. LGTM.

wmoustafa

Thanks @emkornfield for driving this discussion and PR.

site/docs/contribute.md

wmoustafa · 2024-08-26T22:11:10Z

site/docs/contribute.md

+
+Requesting changes on a PR indicates a reviewer believes the PR has merit but still needs issues addressed before merging. If a reviewer believes the change should not be merged at all and there is nothing the author could do to address the reviewers concerns, the reviewer should explicitly state this on the PR. In the rare event that a PR author and reviewers cannot come to a consensus on a PR, the disagreement should be raised to the developer mailing list for further discussion. In this context, a reviewer is anyone leaving comments on the PR including contributors, committers and PMC members.
+
+There are several exceptions to a single committer being able to merge a PR:


Thanks @emkornfield. LGTM.

rdblue · 2024-08-29T15:22:11Z

site/docs/contribute.md

+
+There are several exceptions to a single committer being able to merge a PR:
+
+* Behavioral and functional changes to a specification must go through the [Iceberg improvement proposal](#apache-iceberg-improvement-proposals) before any code can be merged.


Do we really require all changes to the spec to go through an improvement proposal? That doesn't seem right.

I think "Behavioral and function changes" should be going through a more formal process. Not sure exactly what the standards are here?

aokolnychyi · 2024-09-03T18:51:07Z

Thanks @emkornfield and everyone who reviewed! Merged per the dev list vote.

* main: (208 commits) Docs: Fix Flink 1.20 support versions (apache#11065) Flink: Fix compile warning (apache#11072) Docs: Initial committer guidelines and requirements for merging (apache#10780) Core: Refactor ZOrderByteUtils (apache#10624) API: implement types timestamp_ns and timestamptz_ns (apache#9008) Build: Bump com.google.errorprone:error_prone_annotations (apache#11055) Build: Bump mkdocs-material from 9.5.33 to 9.5.34 (apache#11062) Flink: Backport PR apache#10526 to v1.18 and v1.20 (apache#11018) Kafka Connect: Disable publish tasks in runtime project (apache#11032) Flink: add unit tests for range distribution on bucket partition column (apache#11033) Spark 3.5: Use FileGenerationUtil in PlanningBenchmark (apache#11027) Core: Add benchmark for appending files (apache#11029) Build: Ignore benchmark output folders across all modules (apache#11030) Spec: Add RemovePartitionSpecsUpdate REST update type (apache#10846) Docs: bump latest version to 1.6.1 (apache#11036) OpenAPI, Build: Apply spotless to testFixtures source code (apache#11024) Core: Generate realistic bounds in benchmarks (apache#11022) Add REST Compatibility Kit (apache#10908) Flink: backport PR apache#10832 of inferring parallelism in FLIP-27 source (apache#11009) Docs: Add Druid docs url to sidebar (apache#10997) ...

…he#10780)

Strawman proposal for PR merging

9594f37

github-actions bot added the docs label Jul 25, 2024

emkornfield marked this pull request as draft July 25, 2024 06:32

fix link

3c6618d

adutra reviewed Jul 25, 2024

View reviewed changes

Fokko approved these changes Jul 25, 2024

View reviewed changes

site/docs/contribute.md Outdated Show resolved Hide resolved

site/docs/contribute.md Outdated Show resolved Hide resolved

site/docs/contribute.md Outdated Show resolved Hide resolved

site/docs/contribute.md Outdated Show resolved Hide resolved

emkornfield and others added 2 commits July 25, 2024 00:58

Apply suggestions from code review

f6d08df

Co-authored-by: Fokko Driesprong <fokko@apache.org>

revert doc fixes

db01681

Fokko reviewed Jul 25, 2024

View reviewed changes

site/docs/contribute.md Outdated Show resolved Hide resolved

Update site/docs/contribute.md

7cde9a4

Co-authored-by: Fokko Driesprong <fokko@apache.org>

nastra reviewed Jul 25, 2024

View reviewed changes

site/docs/contribute.md Outdated Show resolved Hide resolved

Update site/docs/contribute.md

15b5f1e

Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>

findepi approved these changes Jul 25, 2024

View reviewed changes

jbonofre approved these changes Jul 25, 2024

View reviewed changes

emkornfield changed the title ~~DRAFT: Strawman proposal for PR merging~~ DOC: Strawman proposal for PR merging Jul 25, 2024

emkornfield marked this pull request as ready for review July 25, 2024 23:09

s/or/and/ for large changes

617e2da

jackye1995 reviewed Jul 25, 2024

View reviewed changes

dimas-b reviewed Jul 25, 2024

View reviewed changes

site/docs/contribute.md Outdated Show resolved Hide resolved

Address -1 votes

328ba32

dimas-b approved these changes Jul 26, 2024

View reviewed changes

jackye1995 reviewed Jul 26, 2024

View reviewed changes

site/docs/contribute.md Outdated Show resolved Hide resolved

jackye1995 reviewed Jul 26, 2024

View reviewed changes

site/docs/contribute.md Outdated Show resolved Hide resolved

liurenjie1024 reviewed Jul 26, 2024

View reviewed changes

site/docs/contribute.md Outdated Show resolved Hide resolved

emkornfield added 2 commits July 25, 2024 20:39

Address comments.

067d4e7

type fixes

5d0dd9a

jackye1995 approved these changes Jul 26, 2024

View reviewed changes

rdblue reviewed Jul 29, 2024

View reviewed changes

site/docs/contribute.md Outdated Show resolved Hide resolved

rdblue reviewed Jul 29, 2024

View reviewed changes

emkornfield added 2 commits July 29, 2024 11:29

Remove vote language. Add another sentence about trust in committer j…

dfd3792

…udgement

Add exemption for draft specifications

b9e4cbb

szehon-ho reviewed Jul 29, 2024

View reviewed changes

site/docs/contribute.md Outdated Show resolved Hide resolved

emkornfield added 5 commits July 29, 2024 14:13

change wording on waiting 24 hours to should

b545bd8

remove extra new line

4eba307

clarify draft specifications

ec68f20

clarify indepence of specification

d4a8901

Address more feedback:

b91901a

* clarify committer should not be author. Remove content in favor of links. * Separate and define large changes should be discussed on the mailing list.

aokolnychyi reviewed Aug 6, 2024

View reviewed changes

site/docs/contribute.md Outdated Show resolved Hide resolved

add more explanation of conflict of interest.

77e4c52

dimas-b approved these changes Aug 6, 2024

View reviewed changes

remove 20 file rule

3733fb0

dimas-b approved these changes Aug 9, 2024

View reviewed changes

remove ambigious parts

b093cf8

wmoustafa reviewed Aug 17, 2024

View reviewed changes

aokolnychyi approved these changes Aug 19, 2024

View reviewed changes

advancedxy approved these changes Aug 20, 2024

View reviewed changes

address some comments

82ae0c1

wmoustafa approved these changes Aug 26, 2024

View reviewed changes

emkornfield changed the title ~~DOC: Strawman proposal for PR merging~~ Initial committer guidelines and requirements for merging Aug 28, 2024

flyrain approved these changes Aug 29, 2024

View reviewed changes

amogh-jahagirdar approved these changes Aug 29, 2024

View reviewed changes

rdblue reviewed Aug 29, 2024

View reviewed changes

aokolnychyi merged commit a446164 into apache:main Sep 3, 2024
2 checks passed

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

Docs: Initial committer guidelines and requirements for merging (apac…

e80a97c

…he#10780)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial committer guidelines and requirements for merging #10780

Initial committer guidelines and requirements for merging #10780

emkornfield commented Jul 25, 2024

adutra Jul 25, 2024

emkornfield Jul 25, 2024

jackye1995 Jul 25, 2024

Fokko left a comment

jbonofre commented Jul 25, 2024

emkornfield commented Jul 25, 2024

jackye1995 Jul 25, 2024

jackye1995 Jul 25, 2024 •

edited

Loading

emkornfield Jul 26, 2024 •

edited

Loading

emkornfield Jul 26, 2024

stevenzwu commented Jul 26, 2024

rdblue Jul 29, 2024

emkornfield Jul 29, 2024

emkornfield Jul 29, 2024

jackye1995 Jul 31, 2024

emkornfield Jul 31, 2024

szehon-ho Jul 29, 2024

emkornfield Jul 29, 2024

wmoustafa Aug 17, 2024

aokolnychyi Aug 19, 2024

wmoustafa Aug 20, 2024 •

edited

Loading

emkornfield Aug 26, 2024 •

edited

Loading

wmoustafa Aug 26, 2024

wmoustafa left a comment

wmoustafa Aug 26, 2024

rdblue Aug 29, 2024

emkornfield Aug 29, 2024

aokolnychyi commented Sep 3, 2024


		Requesting changes on a PR indicates a reviewer believes the PR has merit but still needs issues addressed before merging. If a reviewer believes the change should not be merged at all and there is nothing the author could do to address the reviewers concerns, the reviewer should explicitly state this on the PR. In the rare event that a PR author and reviewers cannot come to a consensus on a PR, the disagreement should be raised to the developer mailing list for further discussion. In this context, a reviewer is anyone leaving comments on the PR including contributors, committers and PMC members.

		There are several exceptions to a single committer being able to merge a PR:


		There are several exceptions to a single committer being able to merge a PR:

		* Behavioral and functional changes to a specification must go through the [Iceberg improvement proposal](#apache-iceberg-improvement-proposals) before any code can be merged.

Initial committer guidelines and requirements for merging #10780

Initial committer guidelines and requirements for merging #10780

Conversation

emkornfield commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko left a comment

Choose a reason for hiding this comment

jbonofre commented Jul 25, 2024

emkornfield commented Jul 25, 2024

Choose a reason for hiding this comment

jackye1995 Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

emkornfield Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenzwu commented Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmoustafa Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

emkornfield Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmoustafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aokolnychyi commented Sep 3, 2024

jackye1995 Jul 25, 2024 •

edited

Loading

emkornfield Jul 26, 2024 •

edited

Loading

wmoustafa Aug 20, 2024 •

edited

Loading

emkornfield Aug 26, 2024 •

edited

Loading