Re: [dpdk-dev] Suggestion to improve the code review

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 27-May-20 10:28 AM, Jerin Kollanukkaran wrote:
> I think, original discussion[1] on this topic got lost in GitHub vs current workflow.
> 
> 
> I would like to propose GitHub "CODEOWNERS"[2] _LIKE_ scheme for DPDK workflow.
> 
> Current scheme:
> - When we submit a patch to ml, someone(Tree maintainer[3]) needs to manually
> delegate the patch to Tree maintainer in patchwork.
> - Tree maintainer is not responsible for the review of the patch but only responsible
> for merging _after_ the review. That brings the obvious question on review responsibility.
> 
> 
> Proposed scheme:
> - In order to improve review ownership, IMO, it is better the CI tools delegate
> the patch to the actual maintainer(who is responsible for specific code in MAINTAINERS file)
> - I believe, it provides a sense of ownership, avoids last-minute surprise on
> review responsibility and improve review traceability.
> 
> Implementation of the proposed scheme:
> GitHub provides a bot for CODEOWNERS integration, Similar alternative is possible with
> patchwork with "auto delegation scheme" using the flowing methods:
> 
> a) https://patchwork.readthedocs.io/en/latest/usage/delegation/
> b) https://patchwork.readthedocs.io/en/latest/usage/headers/
> 
> I think, option (a) would be relatively easy to change without introducing the new tools.
> 
> Thoughts?
> 
> [1]
> http://mails.dpdk.org/archives/dev/2020-May/168740.html
> [2]
> https://github.com/zephyrproject-rtos/zephyr/blob/master/CODEOWNERS
> [3]
> https://patchwork.dpdk.org/project/dpdk/
> 

The "which patches should i review first" button is a huge +1000 from 
me, as this has been a big issue i've had with current workflow for a 
long time. Thomas has mentioned "Cc:" as a "fine grained" system to 
assign patches, but the truth is, CC is not a good way of doing it 
because i get CC'd in all kinds of stuff, and the important patches get 
lost.

That said, i don't think it's that easy, because more often than not, 
patches touch a lot of different areas, so a one line change in meson, a 
test and a line in EAL gets half of DPDK maintainers CC'd into the 
patch. I wonder if there is a mechanism for some kind of "threshold" for 
assigning people to the patch - i.e. if a one-liner is half of the 
changes in the patch, then maintainer gets CC'd, but if a one-liner is 
just one of a thousand other unrelated lines, then perhaps there's no 
need to CC the maintainer... or something along those lines :) there's a 
machine learning project in here somewhere :D

-- 
Thanks,
Anatoly