Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better process to avoid duplicate work and resolve technical disagreement #878

Closed
joyeecheung opened this issue Jun 5, 2020 · 7 comments

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Jun 5, 2020

(I am not entirely sure what exactly @jasnell wanted to discuss, feel free to edit the issue if this is not the topic you had in mind).

As discussed in the last TSC meeting (#873), opening this issue so that we can discuss about this asynchronously. For more context, see nodejs/node#32761 and nodejs/node#32984.

Quoting @jasnell in nodejs/node#32761 (comment)

@addaleax ... Before this is abandoned, let's put the issue on the next week @nodejs/tsc agenda to discuss. I was also hoping that more folks would get involved in the discussion and I'm disappointed but not surprised that others didn't jump in but given that we're both NearFormers I didn't want to risk a one sided pile on while technical issues were being worked through. Fwiw, I'm feeling the same frustration about lack of engagement on the quic prs... Which have sat for months with only two tsc members showing much interest outside of you and I.

I understand if you're too personally frustrated with the process to move this forward, I can step in and take it on because I think this work is definitely important. It would just be good to raise the visibility directly with the tsc.

And quoting myself in #868 (comment)

nodejs/node#32761 was closed though the technical disagreement is still not settled (for differences between nodejs/node#32761 and nodejs/node#32984, see the discussions in https://docs.google.com/document/d/15bu038I36oILq5t4Qju1sS2nKudVB6NSGWz00oD48Q8/edit?usp=sharing, would love more input there to break the tie). As @jasnell mentioned in nodejs/node#32761 (comment) I think this reveals that we have some issues with the current process. Some points that I can think of

  • There should've been a better way to communicate about active efforts to avoid duplication of work and competition. In this case there were occasional updates in the tracking issue RFC: speeding up Node.js startup using V8 snapshot node#17058, related PRs and meetings, but those were not visible enough and still lead to conflicts
  • We need more reviewers for certain tasks or certain subsystems, ideally more than two people (to break the tie when there are technical disagreements) and from more than one party (to avoid bias). I don't think it's a good idea to always escalate these to TSC, because not all of us are interested in or familiar with the same part of the code base, and so we don't always get the attention we ask for if the disagreement involves substantial amount of reading or context. Maybe a team + CODEOWNERS structure can help
  • When the change involves substantial amount of work, maybe starting a design doc first to seek high-level and more abstract inputs before jumping in to do the actual implementation would be a good idea. I can see this may be a double edged sword but this works relatively well in V8 (though sometimes prototyping is still necessary to keep the technical discussion realistic)
@gireeshpunathil
Copy link
Member

I should confess that I was able to follow the PR to some extent, but had to leave behind due to its technical depth!

I like the idea of a top-down approach in PRs and reviews to be employed in:

  • large PRs
  • PRs with complex changes
  • PRs with massive refactoring
  • PRs that introduce new protocols, bindings etc.

wherein top-down here means i) presenting a high level objective, ii) an architecture if applicable, iii) design and design considerations. These can come before the PR, so much of the ratification is achieved before we write the first line of code.

@mhdawson
Copy link
Member

I had wondered if something like an rfc process would make sense, although we were not too successful with the earlier enhancement proposal process which we had tried at one point in the past. Maybe if we can make it light weight enough it might work/help.

@gireeshpunathil
Copy link
Member

@mhdawson - do you remember what were the blockers for rfc model? It looks great to me, and I believe it can potentially reduce / eliminate disputes and moreover allows diverse views to converge and improve the overall PR experience.

@richardlau
Copy link
Member

@mhdawson - do you remember what were the blockers for rfc model? It looks great to me, and I believe it can potentially reduce / eliminate disputes and moreover allows diverse views to converge and improve the overall PR experience.

nodejs/node-eps#65 and #335

@gireeshpunathil
Copy link
Member

thanks @richardlau . So keeping aside the fact that most of the bad examples in that repo were reasonably heavy-weight features, I think the pattern reported there could occur for any rfc issue.

So here we are:

  • if a PR is preceded by an rfc, we run the risk of bike-shedding, trolling etc.
  • if not, we run the risk of dispute in the ground

may be we need a middle ground?

@mhdawson
Copy link
Member

mhdawson commented Jan 4, 2021

@jasnell, @joyeecheung I'm thinking this can be closed since we've not discussed/progressed in quite some time. Please let me know if that is not the case, otherwise I'll close in a week or so.

@mhdawson
Copy link
Member

Closing since nobodies chiming in that it needs to stay open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants