Skip to content

Add more documentation on Presto committers#22942

Merged
steveburnett merged 1 commit intoprestodb:masterfrom
tdcmeehan:committers
Jun 17, 2024
Merged

Add more documentation on Presto committers#22942
steveburnett merged 1 commit intoprestodb:masterfrom
tdcmeehan:committers

Conversation

@tdcmeehan
Copy link
Contributor

@tdcmeehan tdcmeehan commented Jun 6, 2024

Description

Within the TSC, we debated on a process to allow people entry into the committers group. The consensus is that we need some basic guidelines, outlined as a proposal here.

Motivation and Context

Better documentation for more project-wide committers.

Impact

More committers in the project.

Test Plan

N/A

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

== NO RELEASE NOTE ==

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Found one link to correct formatting of, no other concerns.

steveburnett
steveburnett previously approved these changes Jun 6, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (doc)

jaystarshot
jaystarshot previously approved these changes Jun 6, 2024
aditi-pandit
aditi-pandit previously approved these changes Jun 6, 2024
CONTRIBUTING.md Outdated

* They have contributed at least one non-trivial change to the project outside of their core area;
* They exercise great judgment (including deferring to others when appropriate);
* They have a curiosity of the codebase (by making code changes or reviewing code outside of their core area);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have a curiosity of the --> are curious about

Frankly, I'd prefer to avoid assigning motivations to people. Either drop this one, or make it more concrete as simply ""They have made code changes or reviewed code in more than one area."

Copy link
Contributor

@rschlussel rschlussel Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is meant to express that someone will look into areas that are unfamiliar to them (e.g learn more about some feature to help with review or fix some bug, or that they make a change in the way that is most maintainable/aligned with the overall architecture and needs of the project even if it requires delving into more unfamiliar territory vs. what's the minimal amount they need to understand to hack some solution together).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this, and while "curiosity" does capture this well, I also agree that it's subjective (we don't want people to think if one is "curious enough").

So I broke it into two bullets, firstly, there are code changes and reviews outside their core area, and secondly, they set a high bar for their own code and for others during code review. I think the combination of these two things will encapsulate what I intended to convey with that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. Did you want to delete this bullet point then? I still find the phrasing "a curiosity of the codebase" a little awkward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, fixed!

CONTRIBUTING.md Outdated
New committers are approved by majority vote of the TSC ([see TSC charter](https://github.com/prestodb/tsc/blob/master/CHARTER.md)). To become a committer, reach out to an [existing TSC member](https://github.com/prestodb/tsc#members) and ask for their feedback on your eligibility (see: [How to become a Presto Committer?](https://github.com/prestodb/presto/wiki/How-to-become-a-Presto-committer%3F)). Note: to expedite the process, consider creating a document that outlines your Github stats, such as the number of reviews, lines of code added, number of PRs, and outlines particularly outstanding code and review contributions. If the TSC member believes you are eligible, they will submit your nomination to a vote by the TSC, typically in the form of a PR that adds your handle to the `CODEOWNERS` file. The process is complete once the PR is merged.
### Baseline expectations from committers

Both module and project committers must demonstrate technical mastery and commitment to the project’s community and growth. In particular, they must show evidence that they are aligned with the goals outlined in [ARCHITECTURE](ARCHITECTURE.md), and demonstrate kindness and professionalism; help others in the project; and work collaboratively as a team.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I wonder whether "growth" is a terminal goal. There's something to be said for mature projects that serve a definite purpose and are stable and maintained over years, but don't grow.

I think Presto still has plenty of room to grow, but there is such a thing as a project that is simply finished.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that growth is not a terminal goal. I think instead, we want diversification, both in terms of the number of key contributors, and the diversity of employers of these key contributors. We need to make sure the project survives specific individuals and specific companies. We do have a diversity of committers compared with some competing projects, but in my opinion it's still not enough.

So, for now, I like "growth", because to achieve this diversity will require growth, but open to your suggestion on how to phrase this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should describe the current state of the Presto project. I agree growth is not a terminal goal, and also that Presto has plenty of room to grow so I think we are a long way away from being a maintained but not-growing project. However, I also see

"I think instead, we want diversification..."

I have a two-part suggestion to consider:

  • Perhaps something similar to this, separating "aligned with the project’s community and growth" from "technical mastery" and grouping it with the other valued characteristics:

    "Both module and project committers must demonstrate technical mastery of at least their core areas. They must also show evidence that they are aligned with the project’s community and growth, including the goals outlined in ARCHITECTURE, demonstrating kindness and professionalism; helping others in the project; and working collaboratively as a team."

    (as an additional suggestion, "including" could be followed by a colon, and a four-item unordered list of "the goals", "demonstrating", "helping", and "working" to emphasize these topics for potential committers to find more easily)

  • If what we want is diversification, let's say so, and add this explanation of diversification and "make sure the project survives specific individuals and specific companies" to ARCHITECTURE.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added that, PTAL!

zhenxiao
zhenxiao previously approved these changes Jun 12, 2024
@tdcmeehan tdcmeehan requested a review from aditi-pandit June 12, 2024 20:46
elharo
elharo previously approved these changes Jun 12, 2024
CONTRIBUTING.md Outdated

* They have contributed at least one non-trivial change to the project outside of their core area;
* They exercise great judgment (including deferring to others when appropriate);
* They have a curiosity of the codebase (by making code changes or reviewing code outside of their core area);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. Did you want to delete this bullet point then? I still find the phrasing "a curiosity of the codebase" a little awkward.

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks! Suggest linking to ARCHITECTURE.

rschlussel
rschlussel previously approved these changes Jun 14, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (doc)

@steveburnett steveburnett merged commit 35eab5d into prestodb:master Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants