Hi Penghui: < All of the discussions here are about the Admin API / CLI changes < he 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. sorry, I didn't express clearly.
> ### 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. maybe we should add a box named `metrics(new metrics added, adding new labels, removing labels etc.)`, Otherwise, [x]metrics will be selected for the metrics bug fix like the schema option > 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. yes, I think we are. Thanks, Bo PengHui Li <peng...@apache.org> 于2022年12月19日周一 16:22写道： > > 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 > > > > > > >>> > > > > > > > > > > > > > > > > > >
