[Gluster-Maintainers] RFC: Suggestions on patches taking more than 10 tries

[Shyam Ranganathan]

On 10/31/2017 01:01 AM, Amar Tumballi wrote:
> Possible reason it could happen, and possible suggestions, feel free to 
> voice your concerns:
> 
> 
>   * If people are doing 'rebase' only to check centos regressions:
>       o then I guess they should be told that they can probably be able
>         to run the tests locally too.
>       o is there a documentation on how to run the test locally? if not,
>         lets write one.

Slightly orthogonal but relevant to the issue raised above,

A rebase or a recheck should have some written justification to 
understand if the contributor is attempting it after an analysis of the 
failure. This helps in understanding that enough diligence is done and 
possibly also raises some alerts for spurious failures that need attention.

fstat [1] helps tracking the various failures and enables us to handle 
failures as a broad aspect, but even then we need the above.

So to your point, yes education is a must, but in addition to the above, 
also educating on leaving behind a comment or two when rechecking would 
be helpful.

[1] 
https://fstat.gluster.org/summary?start_date=2017-10-01&end_date=2017-10-31&branch=master

> 
> 
>   * If 'rebase' happens mostly to resolve merge conflicts:
>       o very probable with patches which are big, and things which touch
>         many files.
>       o then maintainers of the components, or overall maintainers can
>         step up and take call.
>       o A request email for fastening the review process would also help.

I agree on the 3rd bullet. What are the expectations from maintainers in 
the second bullet is not clear to me, can you elaborate? I understand 
this as maintainers can themselves do the conflict merge and hasten the 
process, if so I agree.

> 
> 
>   * If 'rebase' happens because maintainers like more than 50% of the
>     code (ie, idea, logic etc), but there are dis-agreements on few pieces:
>       o Can happen with a new contributor as they would be coming from
>         contributing to different project and the method we follow may
>         be different.
>       o Different maintainers have different opinions on how to use
>         macros, how to define variables etc etc..
>       o In this case, I suggest maintainers can send a message to
>         author, and send an updated patch with their suggestion (with
>         making sure '--author' is set to original author). This can save
>         both the effort of review, and also heart burn of someone not
>         understanding the comments properly.
>       o Documenting that this can happen also means a developer wouldn't
>         feel bad, and would learn from watching the actual diff between
>         his last change, and the change maintainer did.
>       o This also means, a work which may be critical for the project
>         doesn't take very long to land into project. Reminder, stability
>         and progress of the project should be our utmost priority.
>       o Note that I have seen at least 5 such instances where if the
>         reviewer picked up the patch, and worked on it, it could have
>         been faster.
>       o Again, this is not good to repeat always from a developer.
>         Should be OK for a contributor who is less than an year into the
>         component (Considering different maintainers have different
>         choices). Not ideal for someone older than that to the project.
>         But it should be maintainer's call.

I agree to the above, that, maintainers can refresh patches to address 
possible common concerns, or maybe even to show alternate approach in 
areas. Like Jeff stated, we possibly need some 
documentation/clarifications around the same.

> 
> 
> Let me know what you all think? I would also like to talk about it in 
> maintainer's meeting day after.
> 
> Regards,
> Amar
> 
> 
> 
> 
> _______________________________________________
> maintainers mailing list
> maintainers at gluster.org
> http://lists.gluster.org/mailman/listinfo/maintainers
>