Make more client services into startstop.Service
#262
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brandur
commented
Mar 10, 2024
| // | ||
| // Unlike other services, it's given a background context so that it doesn't | ||
| // cancel on normal stops. | ||
| if err := c.monitor.Start(context.Background()); err != nil { //nolint:contextcheck |
There was a problem hiding this comment.
Won't return an error, but like other services, it has the option to do so.
brandur
commented
Mar 10, 2024
| // In case of error, stop any services that might have started. This | ||
| // is safe because even services that were never started will still | ||
| // tolerate being stopped. | ||
| startstop.StopAllParallel(c.services) |
There was a problem hiding this comment.
We now try and stop anything that started even in the event of a start error, which is kind of nice.
Here, we aim to move in a direction where the client can treat most of
its subservices generically by starting/stopping the notifier as a
generic service, along with converting three other smaller services over
to use the `startstop.Service` interface:
* Client monitor. (This ends up cleaning up its internal code
significantly, and has a major improvement on its test coverage and
robustness.)
* The client's statistics logging loop.
* The client's leadership change handling loop.
Both the latter two are very small functions, so I implement the new
function `startstop.StartStopFunc` which similar to `river.WorkFunc`,
provides an easy way to get a small service using only a function, which
keeps the code quite clean.
It's going to take a few more changes to get our full simplification
change, but you can already see the beginnings of it as many services
can be started and stopped in ~one line:
func (c *Client[TTx]) signalStopComplete(ctx context.Context) {
// Wait for producers and elector to exit:
c.wg.Wait()
// Stop all mainline services where stop order isn't important.
startstop.StopAllParallel(c.services)
By the end of it, both the elector and producers will also become
services, and we'll be able to fully eliminate the wait group, along
with the stop channels and the roundabout stop functions like
`signalStopComplete`. The idea is that the client itself will also
become a start/stop service, and itself become protected against being
started multiple times, or potential races where `Start`/`Stop` may be
called by multiple goroutines.
brandur
commented
Mar 10, 2024
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
This all just moves down from above where it was previously in a closure.
brandur
commented
Mar 10, 2024
|
|
||
| shutdownOnce: &sync.Once{}, | ||
| shutdownCh: make(chan struct{}), | ||
| doneCh: make(chan struct{}), |
There was a problem hiding this comment.
Lets us get rid of a whole bunch of vars, which is nice. We add a StartStopStress test case also, so we're now much more certain that this code works than before.
brandur
force-pushed
the
brandur-client-services
branch
from
b1a01f5 to
69ef596
Compare
|
@bgentry I'm sure you're just going to love this one lol, but I promise it's for the greater good. |
brandur
added a commit
that referenced
this pull request
Mar 11, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 11, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 11, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
bgentry
approved these changes
Mar 11, 2024
Actually wasn't too bad at all :) |
Hah, okay that's a relief :) Thanks! |
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
brandur
added a commit
that referenced
this pull request
Mar 12, 2024
) Follow up changes like #253, #262, and #263 to make the producer a start/stop service, giving it a more predictable way to invoke start and stop, making it safer to run and cleaning up caller code where it's used in the client and test cases. With all these changes taken together we'll have every service in the client using the same unified service interface, which will clean up code and let us write some neat utilities to operate across all of them. Aside from that, we clean up the producer in ways to bring it more inline with other code, like making logging uniform and having the constructor return only a `*producer` instead of `(*producer, error)` that needs to be checked despite an error always being indicative of a bug in this context. We expand the test suite, adding tests like (1) verifying that workers are really stopped when `workCtx` is cancelled, (2) verifying that the max worker slots work as expected and that the producer limits its fetches, and (3) start/stop stress. Like with #263, we give the producer a poll only mode, which also gets the full test barrage using a shared test transaction instead of full database pool. Also like #263, this poll only mode is still prospective and not yet put to full use (although it will be soon).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here, we aim to move in a direction where the client can treat most of
its subservices generically by starting/stopping the notifier as a
generic service, along with converting three other smaller services over
to use the
startstop.Serviceinterface:Client monitor. (This ends up cleaning up its internal code
significantly, and has a major improvement on its test coverage and
robustness.)
The client's statistics logging loop.
The client's leadership change handling loop.
Both the latter two are very small functions, so I implement the new
function
startstop.StartStopFuncwhich similar toriver.WorkFunc,provides an easy way to get a small service using only a function, which
keeps the code quite clean.
It's going to take a few more changes to get our full simplification
change, but you can already see the beginnings of it as many services
can be started and stopped in ~one line:
By the end of it, both the elector and producers will also become
services, and we'll be able to fully eliminate the wait group, along
with the stop channels and the roundabout stop functions like
signalStopComplete. The idea is that the client itself will alsobecome a start/stop service, and itself become protected against being
started multiple times, or potential races where
Start/Stopmay becalled by multiple goroutines.