Hi Bo, > yes, need a proposal for Admin API/CLI and metrics changes. but It is > difficult for us to judge whether a PR is a bug fix or needs PIP or > doc/required through automatic detection. We can standardize the > committer review process and increase the description of the review > document, which may achieve better results.
All of the discussions here are about the Admin API / CLI changes The metrics changes are about adding new metrics added, adding new labels, removing labels, etc. It's not about the implementations. A bug fix usually happens to the implementation, not the API definition or metrics definition. I think we can just update the contribution guide https://pulsar.apache.org/contributing/ to make it more clear? The contribution guide can be used by contributors / reviewers. And not only a committer can be a reviewer. For the review process and review document. It can be a separate thread if you have any good ideas you want to share. Thanks, Penghui On Sat, Dec 17, 2022 at 9:16 PM 丛搏 <bog...@apache.org> wrote: > > Is it time to require a proposal for Admin API/CLI and metrics changes? > > yes, need a proposal for Admin API/CLI and metrics changes. but It is > difficult for us to judge whether a PR is a bug fix or needs PIP or > doc/required through automatic detection. We can standardize the > committer review process and increase the description of the review > document, which may achieve better results. > > Thanks, > Bo > > PengHui Li <peng...@apache.org> 于2022年12月17日周六 09:00写道： > > > > I have pushed out a PR to update the PR template and PIP for metrics > changes > > > > https://github.com/apache/pulsar/pull/18961 > > > > PTAL. > > > > Thanks, > > Penghui > > > > On Wed, Dec 7, 2022 at 4:17 PM Haiting Jiang <jianghait...@gmail.com> > wrote: > > > > > +1 for enforcing the PIP procedures. > > > > > > > And the CI can try to add labels `doc required` or `wants/proposal` > > > > according to the list selections. > > > > > > Is it possible that the CI can check if there is a "voted" PIP linking > > > to this PR. > > > And the label can be manually added by committers if the PR author > > > missed checking the boxes. > > > > > > Thanks, > > > Haiting > > > > > > On Wed, Dec 7, 2022 at 4:07 PM PengHui Li <peng...@apache.org> wrote: > > > > > > > > > I agree a proposal would be better before adding a PR. But the > > > > document part must be a part of such a proposal. > > > > > > > > Make sense. It looks like we should have a checklist for the > proposal. > > > > The documentation changes should be listed in the proposal. > > > > > > > > > Can the PR template/GitHub process check that if either the api > changes > > > > and doc-required are checked both are checked with textual > information > > > > provided? > > > > > > > > It's a good idea. > > > > I haven't tried, but it looks like it's possible. > > > > We have this list: > > > > > > > > ``` > > > > ### Does this pull request potentially affect one of the following > parts: > > > > > > > > *If the box was checked, please highlight the changes* > > > > > > > > - [ ] Dependencies (add or upgrade a dependency) > > > > - [ ] The public API > > > > - [ ] The schema > > > > - [ ] The default values of configurations > > > > - [ ] The binary protocol > > > > - [ ] The REST endpoints > > > > - [ ] The admin CLI options > > > > - [ ] Anything that affects deployment > > > > ``` > > > > > > > > And the CI can try to add labels `doc required` or `wants/proposal` > > > > according to the > > > > list selections. > > > > > > > > And we can add `The metrics` item to the list. > > > > > > > > Thanks, > > > > Penghui > > > > > > > > On Wed, Dec 7, 2022 at 1:52 PM Dave Fisher <wave4d...@comcast.net> > > > wrote: > > > > > > > > > > > > > > > > > > > Sent from my iPhone > > > > > > > > > > > On Dec 6, 2022, at 9:45 PM, Yunze Xu > <y...@streamnative.io.invalid> > > > > > wrote: > > > > > > > > > > > > Hi Penghui, > > > > > > > > > > > >> But maybe some are missed. > > > > > > > > > > > > That's the point. Each PR that adds or modifies a metric item > must be > > > > > > labeled with "doc-required" and the related documents should be > > > added. > > > > > > However, these PRs are nearly all labeled with "doc-not-needed". > > > > > > > > > > > > I agree a proposal would be better before adding a PR. But the > > > > > > document part must be a part of such a proposal. > > > > > > > > > > Can the PR template/GitHub process check that if either the api > changes > > > > > and doc-required are checked both are checked with textual > information > > > > > provided? > > > > > > > > > > Best, > > > > > Dave > > > > > > > > > > > > Thanks, > > > > > > Yunze > > > > > > > > > > > >> On Wed, Dec 7, 2022 at 11:48 AM PengHui Li <peng...@apache.org> > > > wrote: > > > > > >> > > > > > >> Hi Yunze, > > > > > >> > > > > > >> All the metrics are listed here > > > > > >> https://pulsar.apache.org/docs/2.10.x/reference-metrics/ > > > > > >> > > > > > >> But maybe some are missed. > > > > > >> > > > > > >> Thanks, > > > > > >> Penghui > > > > > >> > > > > > >> On Wed, Dec 7, 2022 at 11:46 AM Yunze Xu > > > <y...@streamnative.io.invalid> > > > > > >> wrote: > > > > > >> > > > > > >>> I agree. It should have required the PIP. > > > > > >>> > > > > > >>> I have another question. Is there any document to describe > these > > > > > >>> metrics? I think the metrics body should be documented well to > > > avoid > > > > > >>> breaking changes. Some external applications might parse the > > > metrics > > > > > >>> according to a specific structure. > > > > > >>> > > > > > >>> Thanks, > > > > > >>> Yunze > > > > > >>> > > > > > >>> On Wed, Dec 7, 2022 at 11:38 AM PengHui Li <peng...@apache.org > > > > > wrote: > > > > > >>>> > > > > > >>>> Hi all, > > > > > >>>> > > > > > >>>> I would like to start a discussion about requiring a proposal > for > > > > > Admin > > > > > >>>> API/CLI > > > > > >>>> and metrics changes. > > > > > >>>> > > > > > >>>> Here are some recent examples that changed the Admin API but > > > without > > > > > >>>> proposals. > > > > > >>>> I just checked the commit logs. Maybe some have a proposal. > Just > > > > > forgot > > > > > >>> to > > > > > >>>> add > > > > > >>>> the proposal link to the PR. > > > > > >>>> > > > > > >>>> https://github.com/apache/pulsar/pull/18218 > > > > > >>>> https://github.com/apache/pulsar/pull/17153 > > > > > >>>> https://github.com/apache/pulsar/pull/16167 > > > > > >>>> https://github.com/apache/pulsar/pull/14930 > > > > > >>>> https://github.com/apache/pulsar/pull/17337 > > > > > >>>> > > > > > >>>> And here are metrics-related proposals. But looks like we > don't > > > have a > > > > > >>>> clear rule > > > > > >>>> for this part (the proposal is required or not) > > > > > >>>> > > > > > >>>> https://github.com/apache/pulsar/issues/18319 > > > > > >>>> https://github.com/apache/pulsar/issues/18560 > > > > > >>>> > > > > > >>>> As more and more users are using Pulsar in production. > > > > > >>>> But the Admin API changes and metrics changes have > > > > > >>>> not required a proposal. This may pose a risk to users. > > > > > >>>> The proposal will have better visibility, and voting is > required. > > > > > >>>> > > > > > >>>> And actually, all the public API changes are proposals > required. > > > > > >>>> > > > > > >>> > > > > > > > > > https://github.com/apache/pulsar/blob/master/wiki/proposals/PIP.md#when-is-a-pip-required > > > > > >>>> But in fact, this is not strictly enforced. > > > > > >>>> > > > > > >>>> Is it time to require a proposal for Admin API/CLI and metrics > > > > > changes? > > > > > >>>> > > > > > >>>> Thanks, > > > > > >>>> Penghui > > > > > >>> > > > > > > > > > > > > > >
