-
-
Notifications
You must be signed in to change notification settings - Fork 244
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
base: main
Are you sure you want to change the base?
Conversation
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. |
We could. However, looking at pros/cons: Build tagspros:
cons:
multiple branchespros:
cons:
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. |
Co-authored-by: Kévin Dunglas <[email protected]>
Co-authored-by: Kévin Dunglas <[email protected]>
Co-authored-by: Kévin Dunglas <[email protected]>
Fixing fibers and allowing dynamically starting/stopping workers also requires a lot of changes in |
Less messy than doing a complex rebase, IMHO. |
b4eab8e
to
62c8319
Compare
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. |
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. |
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