Topics

Inefficient code reviews


Rami Alshafi
 

Devs,

I have extremely simple code review that I submitted 3 weeks ago. It mainly includes changes in README file, white space modifications (based on previous code reviews) and fixing a minor issue with the sample app SVR database. 

Mats has already reviewed it and we went back and forth over 3 separate patches. I have e-mailed the IoTivity maintainers list and got no response.


We really need to improve the efficiency of code reviews and merging code in order to grow the IoTivity developer community.

My last patch took more than 7 months to be reviewed and finally merged!!

Am I alone in this struggle or is this a common problem among developers?

this is the link to the patch https://gerrit.iotivity.org/gerrit/#/c/26733/

Thanks,

-Rami


Ondrej Tomcik
 

Hello Rami,

You’re not the only one. We had the same issue with the Cloud component. Solution was to take over the responsibility for component – architect and maintainer role.

Who is responsible for a component you upstreamed modifications into? I would write directly them.

 

 

 

Ondrej Tomcik :: KISTLER :: measure, analyze, inovate

 

From: iotivity-dev@... <iotivity-dev@...> On Behalf Of Rami Alshafi
Sent: Donnerstag, 30. August 2018 02:48
To: iotivity-dev@...
Subject: [dev] Inefficient code reviews

 

Devs,

I have extremely simple code review that I submitted 3 weeks ago. It mainly includes changes in README file, white space modifications (based on previous code reviews) and fixing a minor issue with the sample app SVR database. 

Mats has already reviewed it and we went back and forth over 3 separate patches. I have e-mailed the IoTivity maintainers list and got no response.

 

We really need to improve the efficiency of code reviews and merging code in order to grow the IoTivity developer community.

My last patch took more than 7 months to be reviewed and finally merged!!

Am I alone in this struggle or is this a common problem among developers?

this is the link to the patch https://gerrit.iotivity.org/gerrit/#/c/26733/

Thanks,

-Rami


Rami Alshafi
 

Ondrej,

Thank you for the advice and the assurance I am not alone. I am the original author of the sample application that I am trying to maintain which is found in this source path (examples/OCFSecure). 

According to this page there is no maintainer for the examples. This explains the delay as other maintainers would need to take on this additional responsibility. 

If possible, I would love to be the maintainer for the examples/OCFSecure sample application so I can maintain it more frequently and efficiently. I read the project governance and I will need to propose my maintainer candidacy to the IoTivity Steering Group. There is not a mention of how to contact the ISG.

Will you please advice me on how to contact the ISG?

Thanks,

-Rami


From: Ondrej Tomcik <Ondrej.Tomcik@...>
Sent: Thursday, August 30, 2018 1:16:57 AM
To: Rami Alshafi; iotivity-dev@...
Subject: RE: Inefficient code reviews
 

Hello Rami,

You’re not the only one. We had the same issue with the Cloud component. Solution was to take over the responsibility for component – architect and maintainer role.

Who is responsible for a component you upstreamed modifications into? I would write directly them.

 

 

 

Ondrej Tomcik :: KISTLER :: measure, analyze, inovate

 

From: iotivity-dev@... <iotivity-dev@...> On Behalf Of Rami Alshafi
Sent: Donnerstag, 30. August 2018 02:48
To: iotivity-dev@...
Subject: [dev] Inefficient code reviews

 

Devs,

I have extremely simple code review that I submitted 3 weeks ago. It mainly includes changes in README file, white space modifications (based on previous code reviews) and fixing a minor issue with the sample app SVR database. 

Mats has already reviewed it and we went back and forth over 3 separate patches. I have e-mailed the IoTivity maintainers list and got no response.

 

We really need to improve the efficiency of code reviews and merging code in order to grow the IoTivity developer community.

My last patch took more than 7 months to be reviewed and finally merged!!

Am I alone in this struggle or is this a common problem among developers?

this is the link to the patch https://gerrit.iotivity.org/gerrit/#/c/26733/

Thanks,

-Rami


Ondrej Tomcik
 

“Examples maintainer” is very generic in my opinion. Examples should be developed for each component by component developers. It’s not separable.

 

I think your code belongs to the IoTivity Security maintainers group, where active maintainers are (based on wiki)

              Aleksey Volkov, Nathan Heldt-Sheller, Kevin Kane, Greg Zaverucha

 

 

Best Regards

 

 

Ondrej Tomcik :: KISTLER :: measure, analyze, inovate

 

From: rami alshafi <rami_in_portland@...>
Sent: Donnerstag, 30. August 2018 21:42
To: Tomcik Ondrej <Ondrej.Tomcik@...>; iotivity-dev@...
Subject: Re: Inefficient code reviews

 

Ondrej,

Thank you for the advice and the assurance I am not alone. I am the original author of the sample application that I am trying to maintain which is found in this source path (examples/OCFSecure). 

According to this page there is no maintainer for the examples. This explains the delay as other maintainers would need to take on this additional responsibility. 

If possible, I would love to be the maintainer for the examples/OCFSecure sample application so I can maintain it more frequently and efficiently. I read the project governance and I will need to propose my maintainer candidacy to the IoTivity Steering Group. There is not a mention of how to contact the ISG.

Will you please advice me on how to contact the ISG?

Thanks,

-Rami


From: Ondrej Tomcik <Ondrej.Tomcik@...>
Sent: Thursday, August 30, 2018 1:16:57 AM
To: Rami Alshafi; iotivity-dev@...
Subject: RE: Inefficient code reviews

 

Hello Rami,

You’re not the only one. We had the same issue with the Cloud component. Solution was to take over the responsibility for component – architect and maintainer role.

Who is responsible for a component you upstreamed modifications into? I would write directly them.

 

 

 

Ondrej Tomcik :: KISTLER :: measure, analyze, inovate

 

From: iotivity-dev@... <iotivity-dev@...> On Behalf Of Rami Alshafi
Sent: Donnerstag, 30. August 2018 02:48
To: iotivity-dev@...
Subject: [dev] Inefficient code reviews

 

Devs,

I have extremely simple code review that I submitted 3 weeks ago. It mainly includes changes in README file, white space modifications (based on previous code reviews) and fixing a minor issue with the sample app SVR database. 

Mats has already reviewed it and we went back and forth over 3 separate patches. I have e-mailed the IoTivity maintainers list and got no response.

 

We really need to improve the efficiency of code reviews and merging code in order to grow the IoTivity developer community.

My last patch took more than 7 months to be reviewed and finally merged!!

Am I alone in this struggle or is this a common problem among developers?

this is the link to the patch https://gerrit.iotivity.org/gerrit/#/c/26733/

Thanks,

-Rami


Gregg Reynolds
 



On Wed, Aug 29, 2018, 7:47 PM Rami Alshafi <rami_in_portland@...> wrote:

Devs,

I have extremely simple code review that I submitted 3 weeks ago. It mainly includes changes in README file, white space modifications (based on previous code reviews) and fixing a minor issue with the sample app SVR database. 

Mats has already reviewed it and we went back and forth over 3 separate patches. I have e-mailed the IoTivity maintainers list and got no response.


We really need to improve the efficiency of code reviews and merging code in order to grow the IoTivity developer community.

My last patch took more than 7 months to be reviewed and finally merged!!

Am I alone in this struggle or is this a common problem among developers?


It's a general problem. I submitted an absolutely trivial but critical patch when I got started that languished for  months. Thats one reason I no longer bother. With gerrit you have to pick your reviewers, AFAIK. So I'm subscribed to the iotivity list but I'll never know you've submitted a patch if you don't explicitly add me to the reviewer list. You might have better luck notifying the list whenever you submit a patch. You might get lucky!

G


Mats Wichmann
 

On 09/04/2018 02:46 PM, Gregg Reynolds wrote:

It's a general problem. I submitted an absolutely trivial but critical
patch when I got started that languished for months. Thats one reason I no
longer bother. With gerrit you have to pick your reviewers, AFAIK. So I'm
subscribed to the iotivity list but I'll never know you've submitted a
patch if you don't explicitly add me to the reviewer list. You might have
better luck notifying the list whenever you submit a patch. You might get
lucky!
I bet we could come up with some things to improve that. Thinking out loud,

There are groups defined which have a different intent, but could be
used to select reviewers a bit more efficiently. See here:

https://gerrit.iotivity.org/gerrit/#/admin/groups/

some of those are well set up, some are empty, some you could imagine
are not even set up. If the patch affects security, you can select
Security Maintainers as a reviewer, and all the members of that group
will find out.

we're already asked to categorize jira tickets, if Gerrit has that
capability, it could ask the same - and then auto-add the relevant group
to the reviewers. (I said this is thinking aloud - this might not be an
available feature).

We could also have a mailing list for all patch activity, so you find
out even if you're not flagged for a review, might give a chance to
review something that affects you even if the submitter didn't know to
put you there.


George Nash
 

Typically the best way to find who to add to your reviews is to use "git blame" find out who else worked on the file you are modifying. These are the people that know the most about that code. Add them. Many of them are no longer active but it does not hurt to add them as reviewers.

Look at who gave +2 approval to the commits made to that file in the past. Add them.

Look at https://wiki.iotivity.org/projects_and_functions find out who are the maintainers for the area of code you are working on. Add them.

If you are introducing a new sample try talking the individuals in charge of the what you are trying to provide a sample for. We have a lot of unmaintained sample code so getting new samples committed will be really difficult. Rami experienced this with his 7-month delay getting his samples in. Also for new samples it is a better idea to commit them to master not a release branch. Release branches are tested more and in general are less likely to accept code that is not attached to a Jira ticket. Master branch (at least in Iotivity) is used for development.

Rami's current 3 week delay has nothing to do with his commit it is a problem with Jenkins it keeps giving him -1 for things unrelated to his change. He needs to help figure out the cause of the Jenkins failure or work with a maintainer and convince them to override the -1 from Jenkins build. (Overriding the -1 from Jenkins is almost never done because it is too likely to cause problems.) I know Mats has been looking into the Jenkins failure but I don't know if anyone else it working on it.

-----Original Message-----
From: iotivity-dev@lists.iotivity.org [mailto:iotivity-dev@lists.iotivity.org] On Behalf Of Mats Wichmann
Sent: Tuesday, September 4, 2018 2:01 PM
To: iotivity-dev@lists.iotivity.org
Subject: Re: [dev] Inefficient code reviews

On 09/04/2018 02:46 PM, Gregg Reynolds wrote:

It's a general problem. I submitted an absolutely trivial but critical
patch when I got started that languished for months. Thats one reason
I no longer bother. With gerrit you have to pick your reviewers,
AFAIK. So I'm subscribed to the iotivity list but I'll never know
you've submitted a patch if you don't explicitly add me to the
reviewer list. You might have better luck notifying the list whenever
you submit a patch. You might get lucky!
I bet we could come up with some things to improve that. Thinking out loud,

There are groups defined which have a different intent, but could be used to select reviewers a bit more efficiently. See here:

https://gerrit.iotivity.org/gerrit/#/admin/groups/

some of those are well set up, some are empty, some you could imagine are not even set up. If the patch affects security, you can select Security Maintainers as a reviewer, and all the members of that group will find out.

we're already asked to categorize jira tickets, if Gerrit has that capability, it could ask the same - and then auto-add the relevant group to the reviewers. (I said this is thinking aloud - this might not be an available feature).

We could also have a mailing list for all patch activity, so you find out even if you're not flagged for a review, might give a chance to review something that affects you even if the submitter didn't know to put you there.


Mats Wichmann
 

On 09/05/2018 11:27 AM, Nash, George wrote:
Typically the best way to find who to add to your reviews is to use "git blame" find out who else worked on the file you are modifying. These are the people that know the most about that code. Add them. Many of them are no longer active but it does not hurt to add them as reviewers.

Look at who gave +2 approval to the commits made to that file in the past. Add them.

Look at https://wiki.iotivity.org/projects_and_functions find out who are the maintainers for the area of code you are working on. Add them.
all of those are great suggestions in general, but only the third one is
actually "reliable" across the project at the moment, due to us having
lost so many core developers in recent times. If you look at this
analysis that github will do for you on the project mirror:

https://github.com/iotivity/iotivity/graphs/contributors

you'll see that very very few of the people responsible for "most of the
code" in the past are still active.

I'm not criticising, it's just a reality and we have to be clever in
light of that.


Nathan Heldt-Sheller
 

Thanks Mats. These graphs don't really show the primary contributors recently, though, because those are often (mostly?) working on release branches, not master! It's not as grim as it looks... most of the folks on those graphs have been gone a long time, because work on master was superceded by work on release branches years ago.

That said, yes, we do have some areas that don't have clear owners/maintainers. It's something that is a priority to sort out, and we discussed it on today's OSWG call. I have the action item to follow up with prior ISG (IoTivity Steering Group) members to try to improve.

Thanks,
Nathan

-----Original Message-----
From: iotivity-dev@lists.iotivity.org [mailto:iotivity-dev@lists.iotivity.org] On Behalf Of Mats Wichmann
Sent: Wednesday, September 5, 2018 11:45 AM
To: iotivity-dev@lists.iotivity.org
Subject: Re: [dev] Inefficient code reviews

On 09/05/2018 11:27 AM, Nash, George wrote:
Typically the best way to find who to add to your reviews is to use "git blame" find out who else worked on the file you are modifying. These are the people that know the most about that code. Add them. Many of them are no longer active but it does not hurt to add them as reviewers.

Look at who gave +2 approval to the commits made to that file in the past. Add them.

Look at https://wiki.iotivity.org/projects_and_functions find out who are the maintainers for the area of code you are working on. Add them.
all of those are great suggestions in general, but only the third one is actually "reliable" across the project at the moment, due to us having lost so many core developers in recent times. If you look at this analysis that github will do for you on the project mirror:

https://github.com/iotivity/iotivity/graphs/contributors

you'll see that very very few of the people responsible for "most of the code" in the past are still active.

I'm not criticising, it's just a reality and we have to be clever in light of that.


Mats Wichmann
 

On 09/05/2018 01:35 PM, Heldt-Sheller, Nathan wrote:
Thanks Mats. These graphs don't really show the primary contributors recently, though, because those are often (mostly?) working on release branches, not master! It's not as grim as it looks... most of the folks on those graphs have been gone a long time, because work on master was superceded by work on release branches years ago.

That said, yes, we do have some areas that don't have clear owners/maintainers. It's something that is a priority to sort out, and we discussed it on today's OSWG call. I have the action item to follow up with prior ISG (IoTivity Steering Group) members to try to improve.
we merge release-branch commits to master fairly regularly, so they
ought actually to show up in the reports. unless I'm completely
misunderstanding how the report works (afiak it excludes merge commits
themselves, but not the commits that were brought in by the merges).


Nathan Heldt-Sheller
 

Yeah I'm not clear about those reports either... but for example since my name doesn't appear at all and I can think of at least a few commits I've done, it's not showing everything.

Either way as I mentioned, the report is showing several folks who haven't contributed in years, so it's not the case that the active devs have stepped back recently... the report is a little misleading is all :)

Thanks though I do appreciate the analysis!

Nathan

-----Original Message-----
From: Mats Wichmann [mailto:mats@wichmann.us]
Sent: Wednesday, September 5, 2018 12:44 PM
To: Heldt-Sheller, Nathan <nathan.heldt-sheller@intel.com>; iotivity-dev@lists.iotivity.org
Subject: Re: [dev] Inefficient code reviews

On 09/05/2018 01:35 PM, Heldt-Sheller, Nathan wrote:
Thanks Mats. These graphs don't really show the primary contributors recently, though, because those are often (mostly?) working on release branches, not master! It's not as grim as it looks... most of the folks on those graphs have been gone a long time, because work on master was superceded by work on release branches years ago.

That said, yes, we do have some areas that don't have clear owners/maintainers. It's something that is a priority to sort out, and we discussed it on today's OSWG call. I have the action item to follow up with prior ISG (IoTivity Steering Group) members to try to improve.
we merge release-branch commits to master fairly regularly, so they ought actually to show up in the reports. unless I'm completely misunderstanding how the report works (afiak it excludes merge commits themselves, but not the commits that were brought in by the merges).


George Nash
 

That is a report from github. It can only report people that have both a github and gerrit account that share the same email. If you don't have a github account using the same email as you use for gerrit you will not show up in that report. So that report is missing a lot of people.

-----Original Message-----
From: iotivity-dev@lists.iotivity.org [mailto:iotivity-dev@lists.iotivity.org] On Behalf Of Nathan Heldt-Sheller
Sent: Wednesday, September 5, 2018 12:51 PM
To: Mats Wichmann <mats@wichmann.us>; iotivity-dev@lists.iotivity.org
Subject: Re: [dev] Inefficient code reviews

Yeah I'm not clear about those reports either... but for example since my name doesn't appear at all and I can think of at least a few commits I've done, it's not showing everything.

Either way as I mentioned, the report is showing several folks who haven't contributed in years, so it's not the case that the active devs have stepped back recently... the report is a little misleading is all :)

Thanks though I do appreciate the analysis!

Nathan

-----Original Message-----
From: Mats Wichmann [mailto:mats@wichmann.us]
Sent: Wednesday, September 5, 2018 12:44 PM
To: Heldt-Sheller, Nathan <nathan.heldt-sheller@intel.com>; iotivity-dev@lists.iotivity.org
Subject: Re: [dev] Inefficient code reviews

On 09/05/2018 01:35 PM, Heldt-Sheller, Nathan wrote:
Thanks Mats. These graphs don't really show the primary contributors recently, though, because those are often (mostly?) working on release branches, not master! It's not as grim as it looks... most of the folks on those graphs have been gone a long time, because work on master was superceded by work on release branches years ago.

That said, yes, we do have some areas that don't have clear owners/maintainers. It's something that is a priority to sort out, and we discussed it on today's OSWG call. I have the action item to follow up with prior ISG (IoTivity Steering Group) members to try to improve.
we merge release-branch commits to master fairly regularly, so they ought actually to show up in the reports. unless I'm completely misunderstanding how the report works (afiak it excludes merge commits themselves, but not the commits that were brought in by the merges).


Mats Wichmann
 

On 09/06/2018 09:56 AM, Nash, George wrote:
That is a report from github. It can only report people that have both a github and gerrit account that share the same email. If you don't have a github account using the same email as you use for gerrit you will not show up in that report. So that report is missing a lot of people.
aha. I didn't realize it only picked github accounts since every commit
has a committer email I figured it would just use.