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

Add new build tag: newworker #1186

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Add new build tag: newworker #1186

wants to merge 17 commits into from

Conversation

withinboredom
Copy link
Collaborator

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
Owner

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
Collaborator 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
Collaborator

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
Collaborator 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
Collaborator

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
Collaborator 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.

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.

3 participants