Reviewers and assignees of PRs

[Zoltan Haindrich <ki...@rxd.hu>]

Hey all!

I'm happy to see that (I guess) everyone is using the PR based stuff without issues - there are still some flaky stuff from time-to-time; but I feel that patches go in 
faster - and I have a feeling we have more reviewes going on as well - which is awesome!

I've read a bit about github "reviewers" / "assignee" stuff - because it seemed somewhat confusing...
Basically both of them could be a group of users - the meaning of these fields should be filled by the community.
I would like to propose to use the "reviewers" to use it as people from whom reviews might be expected.
And use the assignee field to list those who should approve the change to go in (anyone may add asignees/reviewers)

We sometimes forget PRs and they may become "stale" most of them is just falling thru the cracks...to prevent this the best would be if everyone would self-assign PRs which 
are in his/her area of interest.

There are some times when a give feature needs to change not closely related parts of the codebase - this is usually fine; but there are places which might need "more eyes" 
on reviews.
In the past I was sometimes surprised by some interesting changes in say the thrift api / package.jdo / antlr stuff.

Because the jira title may not suggest what files will be changed - I wanted to find a way to auto add some kind of notifications to PRs.

Today I've found a neat solution to this [1] - which goes a little bit beyond what I anticipated - there is a small plugin which could enable to auto-add reviewers based on 
the changed files (adding a reviewer will also emit an email) - I had to fix a few small issues with it to ensure that it works/etc [2].

I really like this approach beacuase it could enable to change the direction of things - and could enable that contributors doesn't neccessarily need to look for reviewers. 
(but this seems more like just sci-fi right now - lets start small and go from there...)

I propose to collect some globs and reviewers in a google doc before we first commit this file into the repo - so that everyone could add things he/she is interested in.

cheers,
Zoltan

[1] https://github.com/marketplace/actions/auto-assign-reviewer-by-files
[2] https://github.com/kgyrtkirk/auto-assign-reviewer-by-files
[3] https://docs.google.com/document/d/11n9acHby31rwVHfRW4zxxYukymHS-tTSYlJEghZwJaY/edit?usp=sharing

Re: Reviewers and assignees of PRs

[Zoltan Haindrich <ki...@rxd.hu>]

Hey Jagat!

Thank you for your feedback - we should definetly need to improve on these fronts.

I'll get to that when I could - but I'll also answer your questions here as well.

On 6/19/20 3:16 AM, Jagat Singh wrote:
> One thing which needs improvement is updating of Hive Contributors wiki
> with whatever process happens on Github and Build server-side.
> 
> The current confluence is silent on what to expect when we create a PR as a
> contributor, who will review, what will build system do? Where to look for
> errors?

definetly - I've added some parts about opening PR but that's not enough.

I think the "who will review" is a sweet spot already....we have do definetly work on that part.

> 
> Based on my first PR experience, do you manually label PRs as test stable,
> unstable etc? I am not sure if that can be automated if not done already
> along with auto assigning of reviews as you intend to do with this current
> proposal.

no; I don't do it manually :D the job is moving those labels around.
Since adding/removing labels needs access to the repo - and ASF doesn't give this kind of access to non-committers; that have left me to configure my own user to do this.

> I can update a few things based on what I learnt as I recently started
> contributing and I feel these all things are missing. But there are many
> things for which I don't know the answer yet and will appreciate if someone
> experienced update the wiki to add details like above questions.

It would be great if you could help in this - you know; someone new to a project see problems differently - and may ask about stuff which is just become like that.

IIRC there was a thread some time ago about that the way we host our hive.apache.org pages have its days numbered - and we should be moving away from it to something more 
modern - like hugo (or some other alternative). I'm bringing this up here because moving to something like that will place the site inside the repo itself - that could 
enable to review site changes on PRs as well...

I think we could move either parts of the wiki to a '.md' file based world - I think we could start with the contributors guide.

There could be low hanging benefits of doing this - like asking for adjustments to the contribution guide withing a patch which changes the way we test stuff.

cheers,
Zoltan



> Thanks,
> 
> Jagat Singh
> 
> On Thu, 18 Jun 2020 at 20:43, Zoltan Haindrich <ki...@rxd.hu> wrote:
> 
>> Hey Panos!
>>
>> On 6/18/20 11:54 AM, Panos Garefalakis wrote:
>>> My only suggestion would be to make reviewing per package/label instead
>> of
>>> files. This will make the process a bit more clear.
>>
>> we could use path globs to select the files - so it could match on
>> packages as well
>> I've not really used it
>> '**/schq/**'
>>
>>> I recently bumped into this GitHub action that lets you automatically
>> label
>>> PRs based on what paths they modify and could help us towards that goal.
>>>
>>> https://github.com/actions/labeler
>>
>> Sure; we can also have that as well! they may fit for different purposes.
>> Aactually - based on the "absence" of some labels (eg: metastore) we may
>> "skip" some tests.
>>
>> cheers,
>> Zoltan
>>
>>>
>>> Thoughts?
>>>
>>> Cheers,
>>> Panagiotis
>>>
>>> On Thu, Jun 18, 2020 at 10:42 AM Zoltan Haindrich <ki...@rxd.hu> wrote:
>>>
>>>> Hey all!
>>>>
>>>> I'm happy to see that (I guess) everyone is using the PR based stuff
>>>> without issues - there are still some flaky stuff from time-to-time;
>> but I
>>>> feel that patches go in
>>>> faster - and I have a feeling we have more reviewes going on as well -
>>>> which is awesome!
>>>>
>>>> I've read a bit about github "reviewers" / "assignee" stuff - because it
>>>> seemed somewhat confusing...
>>>> Basically both of them could be a group of users - the meaning of these
>>>> fields should be filled by the community.
>>>> I would like to propose to use the "reviewers" to use it as people from
>>>> whom reviews might be expected.
>>>> And use the assignee field to list those who should approve the change
>> to
>>>> go in (anyone may add asignees/reviewers)
>>>>
>>>> We sometimes forget PRs and they may become "stale" most of them is just
>>>> falling thru the cracks...to prevent this the best would be if everyone
>>>> would self-assign PRs which
>>>> are in his/her area of interest.
>>>>
>>>> There are some times when a give feature needs to change not closely
>>>> related parts of the codebase - this is usually fine; but there are
>> places
>>>> which might need "more eyes"
>>>> on reviews.
>>>> In the past I was sometimes surprised by some interesting changes in say
>>>> the thrift api / package.jdo / antlr stuff.
>>>>
>>>> Because the jira title may not suggest what files will be changed - I
>>>> wanted to find a way to auto add some kind of notifications to PRs.
>>>>
>>>> Today I've found a neat solution to this [1] - which goes a little bit
>>>> beyond what I anticipated - there is a small plugin which could enable
>> to
>>>> auto-add reviewers based on
>>>> the changed files (adding a reviewer will also emit an email) - I had to
>>>> fix a few small issues with it to ensure that it works/etc [2].
>>>>
>>>> I really like this approach beacuase it could enable to change the
>>>> direction of things - and could enable that contributors doesn't
>>>> neccessarily need to look for reviewers.
>>>> (but this seems more like just sci-fi right now - lets start small and
>> go
>>>> from there...)
>>>>
>>>> I propose to collect some globs and reviewers in a google doc before we
>>>> first commit this file into the repo - so that everyone could add things
>>>> he/she is interested in.
>>>>
>>>> cheers,
>>>> Zoltan
>>>>
>>>> [1]
>> https://github.com/marketplace/actions/auto-assign-reviewer-by-files
>>>> [2] https://github.com/kgyrtkirk/auto-assign-reviewer-by-files
>>>> [3]
>>>>
>> https://docs.google.com/document/d/11n9acHby31rwVHfRW4zxxYukymHS-tTSYlJEghZwJaY/edit?usp=sharing
>>>>
>>>
>>
> 

Re: Reviewers and assignees of PRs

[Jagat Singh <ja...@gmail.com>]

Hello Zoltan,

One thing which needs improvement is updating of Hive Contributors wiki
with whatever process happens on Github and Build server-side.

The current confluence is silent on what to expect when we create a PR as a
contributor, who will review, what will build system do? Where to look for
errors?

Based on my first PR experience, do you manually label PRs as test stable,
unstable etc? I am not sure if that can be automated if not done already
along with auto assigning of reviews as you intend to do with this current
proposal.

I can update a few things based on what I learnt as I recently started
contributing and I feel these all things are missing. But there are many
things for which I don't know the answer yet and will appreciate if someone
experienced update the wiki to add details like above questions.

Thanks,

Jagat Singh

On Thu, 18 Jun 2020 at 20:43, Zoltan Haindrich <ki...@rxd.hu> wrote:

> Hey Panos!
>
> On 6/18/20 11:54 AM, Panos Garefalakis wrote:
> > My only suggestion would be to make reviewing per package/label instead
> of
> > files. This will make the process a bit more clear.
>
> we could use path globs to select the files - so it could match on
> packages as well
> I've not really used it
> '**/schq/**'
>
> > I recently bumped into this GitHub action that lets you automatically
> label
> > PRs based on what paths they modify and could help us towards that goal.
> >
> > https://github.com/actions/labeler
>
> Sure; we can also have that as well! they may fit for different purposes.
> Aactually - based on the "absence" of some labels (eg: metastore) we may
> "skip" some tests.
>
> cheers,
> Zoltan
>
> >
> > Thoughts?
> >
> > Cheers,
> > Panagiotis
> >
> > On Thu, Jun 18, 2020 at 10:42 AM Zoltan Haindrich <ki...@rxd.hu> wrote:
> >
> >> Hey all!
> >>
> >> I'm happy to see that (I guess) everyone is using the PR based stuff
> >> without issues - there are still some flaky stuff from time-to-time;
> but I
> >> feel that patches go in
> >> faster - and I have a feeling we have more reviewes going on as well -
> >> which is awesome!
> >>
> >> I've read a bit about github "reviewers" / "assignee" stuff - because it
> >> seemed somewhat confusing...
> >> Basically both of them could be a group of users - the meaning of these
> >> fields should be filled by the community.
> >> I would like to propose to use the "reviewers" to use it as people from
> >> whom reviews might be expected.
> >> And use the assignee field to list those who should approve the change
> to
> >> go in (anyone may add asignees/reviewers)
> >>
> >> We sometimes forget PRs and they may become "stale" most of them is just
> >> falling thru the cracks...to prevent this the best would be if everyone
> >> would self-assign PRs which
> >> are in his/her area of interest.
> >>
> >> There are some times when a give feature needs to change not closely
> >> related parts of the codebase - this is usually fine; but there are
> places
> >> which might need "more eyes"
> >> on reviews.
> >> In the past I was sometimes surprised by some interesting changes in say
> >> the thrift api / package.jdo / antlr stuff.
> >>
> >> Because the jira title may not suggest what files will be changed - I
> >> wanted to find a way to auto add some kind of notifications to PRs.
> >>
> >> Today I've found a neat solution to this [1] - which goes a little bit
> >> beyond what I anticipated - there is a small plugin which could enable
> to
> >> auto-add reviewers based on
> >> the changed files (adding a reviewer will also emit an email) - I had to
> >> fix a few small issues with it to ensure that it works/etc [2].
> >>
> >> I really like this approach beacuase it could enable to change the
> >> direction of things - and could enable that contributors doesn't
> >> neccessarily need to look for reviewers.
> >> (but this seems more like just sci-fi right now - lets start small and
> go
> >> from there...)
> >>
> >> I propose to collect some globs and reviewers in a google doc before we
> >> first commit this file into the repo - so that everyone could add things
> >> he/she is interested in.
> >>
> >> cheers,
> >> Zoltan
> >>
> >> [1]
> https://github.com/marketplace/actions/auto-assign-reviewer-by-files
> >> [2] https://github.com/kgyrtkirk/auto-assign-reviewer-by-files
> >> [3]
> >>
> https://docs.google.com/document/d/11n9acHby31rwVHfRW4zxxYukymHS-tTSYlJEghZwJaY/edit?usp=sharing
> >>
> >
>

Re: Reviewers and assignees of PRs

[Zoltan Haindrich <ki...@rxd.hu>]

Hey Panos!

On 6/18/20 11:54 AM, Panos Garefalakis wrote:
> My only suggestion would be to make reviewing per package/label instead of
> files. This will make the process a bit more clear.

we could use path globs to select the files - so it could match on packages as well
I've not really used it
'**/schq/**'

> I recently bumped into this GitHub action that lets you automatically label
> PRs based on what paths they modify and could help us towards that goal.
> 
> https://github.com/actions/labeler

Sure; we can also have that as well! they may fit for different purposes.
Aactually - based on the "absence" of some labels (eg: metastore) we may "skip" some tests.

cheers,
Zoltan

> 
> Thoughts?
> 
> Cheers,
> Panagiotis
> 
> On Thu, Jun 18, 2020 at 10:42 AM Zoltan Haindrich <ki...@rxd.hu> wrote:
> 
>> Hey all!
>>
>> I'm happy to see that (I guess) everyone is using the PR based stuff
>> without issues - there are still some flaky stuff from time-to-time; but I
>> feel that patches go in
>> faster - and I have a feeling we have more reviewes going on as well -
>> which is awesome!
>>
>> I've read a bit about github "reviewers" / "assignee" stuff - because it
>> seemed somewhat confusing...
>> Basically both of them could be a group of users - the meaning of these
>> fields should be filled by the community.
>> I would like to propose to use the "reviewers" to use it as people from
>> whom reviews might be expected.
>> And use the assignee field to list those who should approve the change to
>> go in (anyone may add asignees/reviewers)
>>
>> We sometimes forget PRs and they may become "stale" most of them is just
>> falling thru the cracks...to prevent this the best would be if everyone
>> would self-assign PRs which
>> are in his/her area of interest.
>>
>> There are some times when a give feature needs to change not closely
>> related parts of the codebase - this is usually fine; but there are places
>> which might need "more eyes"
>> on reviews.
>> In the past I was sometimes surprised by some interesting changes in say
>> the thrift api / package.jdo / antlr stuff.
>>
>> Because the jira title may not suggest what files will be changed - I
>> wanted to find a way to auto add some kind of notifications to PRs.
>>
>> Today I've found a neat solution to this [1] - which goes a little bit
>> beyond what I anticipated - there is a small plugin which could enable to
>> auto-add reviewers based on
>> the changed files (adding a reviewer will also emit an email) - I had to
>> fix a few small issues with it to ensure that it works/etc [2].
>>
>> I really like this approach beacuase it could enable to change the
>> direction of things - and could enable that contributors doesn't
>> neccessarily need to look for reviewers.
>> (but this seems more like just sci-fi right now - lets start small and go
>> from there...)
>>
>> I propose to collect some globs and reviewers in a google doc before we
>> first commit this file into the repo - so that everyone could add things
>> he/she is interested in.
>>
>> cheers,
>> Zoltan
>>
>> [1] https://github.com/marketplace/actions/auto-assign-reviewer-by-files
>> [2] https://github.com/kgyrtkirk/auto-assign-reviewer-by-files
>> [3]
>> https://docs.google.com/document/d/11n9acHby31rwVHfRW4zxxYukymHS-tTSYlJEghZwJaY/edit?usp=sharing
>>
> 

Re: Reviewers and assignees of PRs

[Panos Garefalakis <pa...@gmail.com>]

Hey Zoltan,

Thanks for doing this! This is definitely a step towards the right
direction.

My only suggestion would be to make reviewing per package/label instead of
files. This will make the process a bit more clear.
I recently bumped into this GitHub action that lets you automatically label
PRs based on what paths they modify and could help us towards that goal.

https://github.com/actions/labeler

Thoughts?

Cheers,
Panagiotis

On Thu, Jun 18, 2020 at 10:42 AM Zoltan Haindrich <ki...@rxd.hu> wrote:

> Hey all!
>
> I'm happy to see that (I guess) everyone is using the PR based stuff
> without issues - there are still some flaky stuff from time-to-time; but I
> feel that patches go in
> faster - and I have a feeling we have more reviewes going on as well -
> which is awesome!
>
> I've read a bit about github "reviewers" / "assignee" stuff - because it
> seemed somewhat confusing...
> Basically both of them could be a group of users - the meaning of these
> fields should be filled by the community.
> I would like to propose to use the "reviewers" to use it as people from
> whom reviews might be expected.
> And use the assignee field to list those who should approve the change to
> go in (anyone may add asignees/reviewers)
>
> We sometimes forget PRs and they may become "stale" most of them is just
> falling thru the cracks...to prevent this the best would be if everyone
> would self-assign PRs which
> are in his/her area of interest.
>
> There are some times when a give feature needs to change not closely
> related parts of the codebase - this is usually fine; but there are places
> which might need "more eyes"
> on reviews.
> In the past I was sometimes surprised by some interesting changes in say
> the thrift api / package.jdo / antlr stuff.
>
> Because the jira title may not suggest what files will be changed - I
> wanted to find a way to auto add some kind of notifications to PRs.
>
> Today I've found a neat solution to this [1] - which goes a little bit
> beyond what I anticipated - there is a small plugin which could enable to
> auto-add reviewers based on
> the changed files (adding a reviewer will also emit an email) - I had to
> fix a few small issues with it to ensure that it works/etc [2].
>
> I really like this approach beacuase it could enable to change the
> direction of things - and could enable that contributors doesn't
> neccessarily need to look for reviewers.
> (but this seems more like just sci-fi right now - lets start small and go
> from there...)
>
> I propose to collect some globs and reviewers in a google doc before we
> first commit this file into the repo - so that everyone could add things
> he/she is interested in.
>
> cheers,
> Zoltan
>
> [1] https://github.com/marketplace/actions/auto-assign-reviewer-by-files
> [2] https://github.com/kgyrtkirk/auto-assign-reviewer-by-files
> [3]
> https://docs.google.com/document/d/11n9acHby31rwVHfRW4zxxYukymHS-tTSYlJEghZwJaY/edit?usp=sharing
>

Re: Reviewers and assignees of PRs

[Zoltan Haindrich <ki...@rxd.hu>]

Hey All!

After jumping over some further requirements/etc I was able to make this work and merge it!
It already found some PRs which are changing the parser/thrift api - which I would have missed otherwise.
I hope that this could help us increasing our PR review rate. I would like to suggest to all committers to add some rules to the .github/assign-by-files.yml file.
It requires the assignee to be the member of the "hive-committers" github group.

cheers,
Zoltan


On 12/11/20 12:00 PM, Zoltan Haindrich wrote:
> Hey All!
> 
> I've prepared the things need for this a long time ago - I've only opened the PR now...
> 
> If you would like to extend the assign-by-files - please either leave a comment on the PR ; or use the "Edit file" option on github to add your changes!
> 
> https://github.com/apache/hive/pull/1767/files
> 
> cheers,
> Zoltan
> 
> On 6/18/20 11:41 AM, Zoltan Haindrich wrote:
>> Hey all!
>>
>> I'm happy to see that (I guess) everyone is using the PR based stuff without issues - there are still some flaky stuff from time-to-time; but I feel that patches go in 
>> faster - and I have a feeling we have more reviewes going on as well - which is awesome!
>>
>> I've read a bit about github "reviewers" / "assignee" stuff - because it seemed somewhat confusing...
>> Basically both of them could be a group of users - the meaning of these fields should be filled by the community.
>> I would like to propose to use the "reviewers" to use it as people from whom reviews might be expected.
>> And use the assignee field to list those who should approve the change to go in (anyone may add asignees/reviewers)
>>
>> We sometimes forget PRs and they may become "stale" most of them is just falling thru the cracks...to prevent this the best would be if everyone would self-assign PRs 
>> which are in his/her area of interest.
>>
>> There are some times when a give feature needs to change not closely related parts of the codebase - this is usually fine; but there are places which might need "more 
>> eyes" on reviews.
>> In the past I was sometimes surprised by some interesting changes in say the thrift api / package.jdo / antlr stuff.
>>
>> Because the jira title may not suggest what files will be changed - I wanted to find a way to auto add some kind of notifications to PRs.
>>
>> Today I've found a neat solution to this [1] - which goes a little bit beyond what I anticipated - there is a small plugin which could enable to auto-add reviewers based 
>> on the changed files (adding a reviewer will also emit an email) - I had to fix a few small issues with it to ensure that it works/etc [2].
>>
>> I really like this approach beacuase it could enable to change the direction of things - and could enable that contributors doesn't neccessarily need to look for 
>> reviewers. (but this seems more like just sci-fi right now - lets start small and go from there...)
>>
>> I propose to collect some globs and reviewers in a google doc before we first commit this file into the repo - so that everyone could add things he/she is interested in.
>>
>> cheers,
>> Zoltan
>>
>> [1] https://github.com/marketplace/actions/auto-assign-reviewer-by-files
>> [2] https://github.com/kgyrtkirk/auto-assign-reviewer-by-files
>> [3] https://docs.google.com/document/d/11n9acHby31rwVHfRW4zxxYukymHS-tTSYlJEghZwJaY/edit?usp=sharing

Re: Reviewers and assignees of PRs

[Zoltan Haindrich <ki...@rxd.hu>]

Hey All!

I've prepared the things need for this a long time ago - I've only opened the PR now...

If you would like to extend the assign-by-files - please either leave a comment on the PR ; or use the "Edit file" option on github to add your changes!

https://github.com/apache/hive/pull/1767/files

cheers,
Zoltan

On 6/18/20 11:41 AM, Zoltan Haindrich wrote:
> Hey all!
> 
> I'm happy to see that (I guess) everyone is using the PR based stuff without issues - there are still some flaky stuff from time-to-time; but I feel that patches go in 
> faster - and I have a feeling we have more reviewes going on as well - which is awesome!
> 
> I've read a bit about github "reviewers" / "assignee" stuff - because it seemed somewhat confusing...
> Basically both of them could be a group of users - the meaning of these fields should be filled by the community.
> I would like to propose to use the "reviewers" to use it as people from whom reviews might be expected.
> And use the assignee field to list those who should approve the change to go in (anyone may add asignees/reviewers)
> 
> We sometimes forget PRs and they may become "stale" most of them is just falling thru the cracks...to prevent this the best would be if everyone would self-assign PRs which 
> are in his/her area of interest.
> 
> There are some times when a give feature needs to change not closely related parts of the codebase - this is usually fine; but there are places which might need "more eyes" 
> on reviews.
> In the past I was sometimes surprised by some interesting changes in say the thrift api / package.jdo / antlr stuff.
> 
> Because the jira title may not suggest what files will be changed - I wanted to find a way to auto add some kind of notifications to PRs.
> 
> Today I've found a neat solution to this [1] - which goes a little bit beyond what I anticipated - there is a small plugin which could enable to auto-add reviewers based on 
> the changed files (adding a reviewer will also emit an email) - I had to fix a few small issues with it to ensure that it works/etc [2].
> 
> I really like this approach beacuase it could enable to change the direction of things - and could enable that contributors doesn't neccessarily need to look for reviewers. 
> (but this seems more like just sci-fi right now - lets start small and go from there...)
> 
> I propose to collect some globs and reviewers in a google doc before we first commit this file into the repo - so that everyone could add things he/she is interested in.
> 
> cheers,
> Zoltan
> 
> [1] https://github.com/marketplace/actions/auto-assign-reviewer-by-files
> [2] https://github.com/kgyrtkirk/auto-assign-reviewer-by-files
> [3] https://docs.google.com/document/d/11n9acHby31rwVHfRW4zxxYukymHS-tTSYlJEghZwJaY/edit?usp=sharing