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

gov: team relationship #122

Merged
merged 18 commits into from
Jun 17, 2021
Merged

gov: team relationship #122

merged 18 commits into from
Jun 17, 2021

Conversation

tisonkun
Copy link
Contributor

@tisonkun tisonkun commented Jun 4, 2021

Signed-off-by: tison wander4096@gmail.com

close #121

description

These files describe relationship in member : team : repository. They themselves do not take effect but for textual display.

Since our permissions are flatten into repository level. @hi-rustin and I tend to use GitHub team to maintain the permission and leave the bot simply respect GitHub permission. See also #123

issues

There are two repositories those do not appear in these files: community, rust-prometheus.

For community, its permission is to all maintainers only.

For rust-prometheus, its development is with collaboration with guys pay less attention to TiKV and not yet be included in previous SIG model.

I'd like to listen to @BusyJay @breeswish @lucab @mxinden on suggestions. Personally I'm thinking of transfer to prometheus community and retain current permissions.

@ghost

This comment has been minimized.

Signed-off-by: tison <wander4096@gmail.com>
Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Thank you!
I think it should be projects/xxxx. also we might be better off changing it to teams? Because we'll be using GitHub teams to control permissions, so it'll be easier for people to understand?

@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 4, 2021

Hi @hi-rustin ! Thanks for your comment.

I have no objection on "team" but it is no the term in previous discussion. I'd like to see more feedback.

@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 4, 2021

For rust-prometheus repository, with a discussion on Slack with @lucab we agree on that it is better to include rust-prometheus as a team (project) under TiKV community.

Then the question is who are the collaborators of that repository so that we can formalize one file to describe it.

@breeswish @lucab @mxinden do you have an idea? I'd try to figure out the collaborators list but it helps if you can say someone who you think should be a committer / maintainer of the repository.

@@ -0,0 +1,31 @@
{
"maintainer": [
Copy link
Member

Choose a reason for hiding this comment

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

Given that PD and TiKV can't be used alone and PD committers are also actively updating TiKV, I suggest to merge pd project and tikv project.

/cc @nolouch

Copy link
Contributor

Choose a reason for hiding this comment

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

There are many PD contributors that are not updating TiKV and PD can run with unistore or TiFlash (after it supports direct write) and there are proposals not TiKV related.

Copy link
Member

Choose a reason for hiding this comment

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

There are many PD contributors that are not updating TiKV

You can find that all PD maintainers, committers have updated TiKV except qiuyesuifeng.

PD can run with unistore or TiFlash (after it supports direct write)

Unfortunately, unistore is not expected to be used in production, and it's purpose is to provide better optimization direction for TiKV. TiFlash is using TiKV to communicate with PD.

The main goal is to have a role that can decide from the global view and choose the design that best suit the evolution of product.

Choose a reason for hiding this comment

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

TiFlash is mainly using the Raft layer of TiKV but not the whole TiKV.

IMO, PD provides the ability to schedule data partition (called Region) under multi-raft, which is a separate layer from TIKV, a specific key-value storage implementation.

Copy link
Member

Choose a reason for hiding this comment

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

TiFlash is mainly using the Raft layer of TiKV but not the whole TiKV.

This doesn't change the fact that PD is talking to TiKV instead of Tiflash.

IMO, PD provides the ability to schedule data partition (called Region) under multi-raft...

It's not true. PD's behavior is tightly bound to the behavior of TiKV. Of cause you can simulate a TiKV using the same protocols, but its design is not for general KV storage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 the proposal does not necessarily require separation, I would like to make one RFC for one feature, I think put them in one org is accepted. such as tikv/rfcs already has 0013-schedule-limit.md it only relative PD, and 0054-joint-consensus.md relative two project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nolouch I think I get your point. We can label an RFC with related projects and require at lease X (maintainer|committer) approval of each project to get the RFC merged.

Copy link
Contributor Author

@tisonkun tisonkun Jun 15, 2021

Choose a reason for hiding this comment

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

Ping @BusyJay we may think it as consensus reached to leave TiKV & PD team as is if there is no more objection tomorrow (2021-06-16). We can discuss later on the description how to accept an RFC. Roughly we tag RFC with team label and require X committers of each team give LGTM.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM

project/raft/project.json Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot requested a review from nolouch June 7, 2021 07:40
Signed-off-by: tison <wander4096@gmail.com>
project/pd/project.json Outdated Show resolved Hide resolved
project/pd/project.json Outdated Show resolved Hide resolved
project/pd/project.json Outdated Show resolved Hide resolved
project/pd/project.json Outdated Show resolved Hide resolved
project/raft/project.json Outdated Show resolved Hide resolved
project/tikv/project.json Outdated Show resolved Hide resolved
tisonkun and others added 5 commits June 8, 2021 11:49
Co-authored-by: 二手掉包工程师 <rustin.liu@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Co-authored-by: 二手掉包工程师 <rustin.liu@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Co-authored-by: 二手掉包工程师 <rustin.liu@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Co-authored-by: 二手掉包工程师 <rustin.liu@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

I still want to use the concept of teams rather than projects.
This will help people understand the new governance structure and associated permissions.

reset LGTM.

project/tikv/project.json Outdated Show resolved Hide resolved
@tisonkun
Copy link
Contributor Author

tisonkun commented Jun 8, 2021

I still want to use the concept of teams rather than projects.
This will help people understand the new governance structure and associated permissions.

reset LGTM.

@hi-rustin I'd suggest you comment on #118 since we define the term there. If there is no objection or we reach a consensus after 72 hours, i.e., this Friday 2021-06-11, I'm gonna update all description on weekend.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Copy link
Contributor Author

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Propose to promote @zhouqiang-cl as a committer according to his work on release these projects. Also the current situation is he already has write permission for these repository to push release process. We should include it in our gov model.

project/pd/project.json Outdated Show resolved Hide resolved
project/prometheus/project.json Outdated Show resolved Hide resolved
project/raft/project.json Outdated Show resolved Hide resolved
project/tikv/project.json Outdated Show resolved Hide resolved
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun changed the title gov: project relationship gov: team relationship Jun 14, 2021
Signed-off-by: tison <wander4096@gmail.com>
team/pd/team.json Outdated Show resolved Hide resolved
Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Thanks!

LGTM.

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • hi-rustin

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 14, 2021
Signed-off-by: tison <wander4096@gmail.com>
@Rustin170506
Copy link
Member

/merge

/hold

If you think we're ready to merge it now, please feel free to /unhold.

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 14, 2021
@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 4ff15e6

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 14, 2021
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

lgtm

@ti-chi-bot
Copy link
Member

@nolouch: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Contributor Author

Thanks to @hi-rustin 's excellent work ti-community-infra/tichi#597 and with the consensus reached at here we are going to apply the change of community governance in days.

@tisonkun
Copy link
Contributor Author

/unhold

@ti-chi-bot ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 17, 2021
@tisonkun
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

@tisonkun: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot ti-chi-bot merged commit e7679a4 into tikv:master Jun 17, 2021
@tisonkun tisonkun deleted the relationship branch June 18, 2021 06:45
@tisonkun
Copy link
Contributor Author

Hi audience of this thread. We are going to apply the authentication rules proposed in #123 with ti-community-infra/configs#358 . Please be aware and report on #123 if you found any bad case.

ti-chi-bot pushed a commit to ti-community-infra/configs that referenced this pull request Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Associate every repository with one and one and only one team