Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PIP-265: PR-based system for managing and reviewing PIPs #20207

Closed
asafm opened this issue Apr 30, 2023 · 5 comments
Closed

PIP-265: PR-based system for managing and reviewing PIPs #20207

asafm opened this issue Apr 30, 2023 · 5 comments
Assignees
Labels

Comments

@asafm
Copy link
Contributor

asafm commented Apr 30, 2023

Background

In the last 2 months, I've increased my PIP review time (I do it in cycles), and reviewed quite a few PIPs.

My conclusion as a result of that:

It's nearly impossible to review PIPs using a mailing list.
We must fix it soon.

Why?

  1. Very hard to review using email.

    Let's say you review the PIP and find 10 issues. Once you quote and comment on those ten points, you basically started 10 threads of conversations.
    After 2-3 ping pongs with quotes of quotes of quotes, it takes you forever to read each thread properly. You find yourself doing CTRL-F to search to find your original quote, and reply. Load it up again in your head, switching to the PIP tab to read it again.
    After 10 ping pongs, it becomes almost an impossible mission.

    I can say I'm 75% tired just from the process, not from the review itself.

  2. It's non-collaborative by design.
    After 10 ping pongs, the ability of someone to come and join the discussion is 0. They need to go through so many replies, which are half quotes, find the original reply, and browse to the PIP.
    It's no wonder people drop off the PIP review once we cross 5–6 replies.
    It's no wonder, nobody joins after 10 replies.

  3. It's not open to the public. Non-collaborative.
    You can't just get a link, and join the review. Not only because of (1) and (2). You need to join the mailing list. You don't get the past emails to reply. Just joining the list is a high enough bar for many people.
    I personally participated in review of proposals in OpenTelemetry in the last 6 months, even though I'm just an occasional user. Why? They were conducted on GitHub PR, so it was easy for me - click a link and reply.

  4. All over the place
    Sometimes people comment on the GitHub issue.
    Sometimes on the mailing list.
    Not a single place.

  5. No history.
    Ok, finally the author was convinced. I can't see just the changes. They need to explicitly tell me something was changed.

  6. Delete All.
    They can go crazy, after 1 year, edit the GitHub issue, delete all the text and write "Kafka is the king". No history, nobody can stop them. It's their issue.

  7. Show me all the approved PIPs
    Hard to track it today, hard to keep up to date.

  8. Resolved comments
    Even though you managed to read all 35 replies so far, in reply 36 you see the author agreed to all 8 out of 10 suggestions. You have no idea of knowing that in advance. You just wasted 1 hour.

Proposal

PR is the main tool we have that allows multiple threaded discussion.
Git provides history. You can't delete it without approval from PMC members.

  1. We'll create a folder named "pip" in the pulsar main repo. It will contain one markdown file for each PIP. The file will be named pip-xxx.md. We will write below how to obtain xxxbefore you start.

  2. To create a PIP, you grab pip/template.md and use it to compose your file in the pip folder.

  3. You submit this file as a PR named "PIP-xxx: {short description}".

  4. You create "[DISCUSS] PIP-xxx: {short description}" in the DEV mailing list and refer people to your PR, with short text explaining the gist of it.

  5. People discuss using PR comments, each is its own-threaded comment. General comments can be made both as replies in the mailing list or as general comment in the PR. After 10 PIPs in this way we’ll be able to see what people gravitate towards and what’s more convenient and consider refining that.

  6. Comment was done? They resolve it. This way you see what the pending discussions are at a glance.

  7. Reached consensus? Good. Send "[VOTE] PIP-xxx: short description" on DEV mailing list,

  8. Following vote process as described before:

    Everyone is welcome to vote on the proposal, though only the vote of the PMC members will be considered binding. It is required to have a lazy majority of at least 3 binding +1s votes. The vote should stay open for at least 48 hours.

  9. PIP approved? Awesome. Push commit with link to the vote on mailing list.
    A PMC member will merge it.
    Merge == approved.
    PMC members can add a PIP label.

  10. Rejected? All good. Close the PR.
    Closed == Rejected.
    It can't be deleted. All comments are still there.

There will be README in the pip folder containing:

  1. The process description
  2. Link to find accepted proposal PRs
  3. Link to find rejected proposal (PRs)
  4. Historical reference to PIPs prior to this proposal (where to find them).

This will replace existing wiki/proposal/PIP.mdwhich will be merged into the README.
All links pointing to that PIP.md file (from wiki, pulsar-site) will be amended to point to the new README file.

Grabbing PIP number

Before you start, you search Pull Requests with label PIP in GitHub (is:pr "PIP-" in:title)
Take the biggest number and add 1.
It is super rare to have two people create PR at the same time.

Show me all approved PIPs

Option 1: Search merged PRs labeled PIP.
Option 2: Look at "pip" folder

Show me rejected PIPs

Search closed PRs with "PIP-" in title, or labeled PIP.

Handling images

Since documents will now reside as files in git, we need to avoid large image files.
Hence, we'll specify to author that images needs to be created using mermaidJS diagram language, which GitHub supports rendering. It covers 99% of the cases. For the 1% case, they can use small file size format SVG, and make sure the file is 1k-5k size.

Alternatives

Use PR for voting instead of mailing list

Processed are best changes one step at a time. This can be considered in the future, but out of scope for this proposal. For now, we’ll keep the voting as it is today: in the VOTE mail in the DEV mailing list.

General discussion should be only in the mailing list

Judging from PIPs I read, I would say the majority of the feedback is not in the scope of the PIP, but in the scope of certain section / part of the PIP. If 90% of the comments already transpire in the PR, I don’t think it will benefit for the mere 10%.

Also, human beings are hard at using two systems at the same time :)

Another big advantage on keeping the general comments in the PR is that it’s public and everybody can pitch in - For example, Eron Wright was invited to help on a PIP for Open ID Connect (Michael) by team members. If the barrier was joining the mailing list, we might not get that valuable comment.

Yet, I think it’s best to do changes incrementally, and use both the mailing list and PR and see what people choose organically.

A separate repo for “pips”

It’s possible, of course, but I fear the following:

  • It’s yet another repo to clone and search. Majority of PIPs are Pulsar related and the majority of Pulsar contributors have that cloned, used and up to date weekly / daily. It would create less friction if it is on main repo. You need to search? Pulsar is already there, ready.
  • a Previous discussion long time ago had many decision points which eventually “killed” the initiative.

We can always move it if it really causes a pain point to many people.

Keeping a README with file name and proposal description

We have that today, in the wiki, but as can be seen, people forget to update it. We can always add such a list in the future. Today the list is the files in the directory and link to reject PIPs (Search of closed PRs with label of PIP)

Links

Discussion leading to this PIP: https://lists.apache.org/thread/5kpddlfh5xdbsjmv47ymnk3z6wd92jbh
DEV email: https://lists.apache.org/thread/2624hnox7kb7p4k5qhfpr4rkt6yh5jp1
VOTE email: https://lists.apache.org/thread/lsojtpc4s86omwtly1gxczs3tbbd0wnz

@asafm asafm added the type/PIP label Apr 30, 2023
@asafm
Copy link
Contributor Author

asafm commented May 22, 2023

Plan

@michaeljmarshall
Copy link
Member

We will write below how to obtain xxx before you start.

Did we figure this part out?

Remove issue type PIP

I think it might make sense to keep the PIP type for a brief period with explicit instructions on how to follow the new process.

@asafm
Copy link
Contributor Author

asafm commented May 28, 2023

@michaeljmarshall Check out the PR: #20418

@asafm
Copy link
Contributor Author

asafm commented May 28, 2023

Regarding, I'll modify the PIP issue template as well @michaeljmarshall

@asafm
Copy link
Contributor Author

asafm commented May 30, 2023

All done 🎉

@asafm asafm closed this as completed May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 participants