Skip to content

Conversation

@withinboredom
Copy link
Member

This is the bare minimum to allow compilation that removes worker.go by adding a new build tag: newworker.

This allows us to refactor and merge major changes to workers without breaking existing workers and work together by merging "dead code". It also defines NEW_WORKER in C, so that we can do #ifdef's depending on the worker running.

Once we are ready, we can delete the build tag and "old workers", making "new workers" the default implementation.

see: #1175

@dunglas
Copy link
Member

dunglas commented Nov 21, 2024

Couldn't we just use a new branch where we'll merge the related PR and rebase regularly against main? I'm not fond of having both implementations in the same code base.

@withinboredom
Copy link
Member Author

We could. However, looking at pros/cons:

Build tags

pros:

  • any changes we make, can be done to both while it is in our heads, without much context/branch switching
  • worst case, we have to do work twice anyway; this just makes it explicit
  • the person making the change is the one who reconciles it for both versions
  • no merge conflicts
  • can see the consequence of a change to both implementations immediately without context/branch switching

cons:

  • two concurrent implementations
  • extra complexity

multiple branches

pros:

  • only have to focus on the implementation you are changing
  • can make drastic, sweeping changes
  • easier to wrap your head around

cons:

  • the person resolving a merge conflict may not be the person who created it (or properly understands the context)
  • bad merges (duplicated lines, missing lines, etc.)
  • duplicate effort if a change needs to be made, requiring a context/branch switch

For a small team, on a young codebase (where a bugfix can potentially end up with a refactor), I'd suggest the build tag approach so that both implementations can be changed together. For a bigger team, or a more mature codebase, I'd suggest the branch approach.

@AlliBalliBaba
Copy link
Contributor

can make drastic, sweeping changes

Fixing fibers and allowing dynamically starting/stopping workers also requires a lot of changes in frankenphp.c and other files. That makes keeping around both implementations too messy IMO

@withinboredom
Copy link
Member Author

Less messy than doing a complex rebase, IMHO.

Base automatically changed from refactor/workers2 to main November 23, 2024 10:29
An error occurred while trying to automatically change base from refactor/workers2 to main November 23, 2024 10:29
@AlliBalliBaba
Copy link
Contributor

Not necessarily less messy, you still have to implement every new change twice instead of resolving the merge conflict. Having the 'new worker' tag would ultimately also require running the pipeline twice, once with the tag and once without it.

@withinboredom
Copy link
Member Author

For these kinds of merge conflicts, you'd likely have to do the work twice anyway (or more). It just becomes a question of doing it twice while it is in your head, or doing it in a random past commit(s) of someone else's changes.

For CI, we don't have to run the actions until we are ready; we can just run the tests locally.

@dunglas
Copy link
Member

dunglas commented Dec 17, 2024

Can this one be closed now that #1137 has been merged?

@withinboredom withinboredom deleted the refactor/workers3 branch December 18, 2024 00:21
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

Successfully merging this pull request may close these issues.

4 participants