Use BasicService in state_replication.go for replication loop. #3930

stevesg · 2021-03-08T20:30:52Z

What this PR does:
Use BasicService in state_replication.go for replication loop.

Checklist

Tests updated
~~Documentation added~~
~~CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]~~

stevesg

Having done this I mentioned to @pstibrany whether we should convert the whole Alertmanager to be a Service, but that would be a much bigger commit, and has more risk than this change which is reasonably isolated.

stevesg · 2021-03-08T20:32:31Z

pkg/alertmanager/alertmanager.go

This seemed like the least-worst option for the time being but open to suggestions, alternatives being:

Add Stop() method to State interface - Means we would need to a Stop method to Peer in Prometheus

Storing the service separately as stateService or something, again.

pkg/alertmanager/alertmanager.go

Signed-off-by: Steve Simpson <[email protected]>

pracucci

Good job! LGTM (modulo a couple of nits). I left few comments about the design evolution, but I would keep it for a follow-up if we agree on it.

pkg/alertmanager/alertmanager.go

pracucci · 2021-03-09T11:24:04Z

pkg/alertmanager/alertmanager.go

 		am.dispatcher.Stop()
 	}

+	if service, ok := am.state.(*state); ok {


I think it's more clean if you cast to the Service interface.

Suggested change

if service, ok := am.state.(*state); ok {

if service, ok := am.state.(services.Service); ok {

That being said, the next step of refactoring I see (but I would do it in a separate PR) is to require am.state to implement the services.Service interface. We can wrap cfg.Peer into our own struct which implements services.Service (using services.NewIdleService()) and add services.Service to NilPeer too. This way we keep all implementations consistent.

Thoughts @stevesg @pstibrany ?

Seems reasonable. We should consider how to call Settle for the non-sharded case too, ideally that would be consistent (the Service wrapper for the Peer wrapper does the settling in starting, as for the replication).

The alternative to the road we've started on, is to have a single "PeerService" which wraps any type of Peer, though that would then need to be extended with some sort of "run" function, so you probably end back at making them all services...

so you probably end back at making them all services...

I think having separate services would keep things cleaner at the end. Code duplication should be minimal.

pkg/alertmanager/alertmanager.go

pracucci · 2021-03-09T11:28:24Z

pkg/alertmanager/state_replication.go

 }

-func (s *state) handleBroadcasts() {
+func (s *state) running(ctx context.Context) error {


Let me leave some comments here, but not related to running(). Something to discuss about (but I would keep it separated from this PR):

Why not doing the Settle() in the service starting?

If we do There is no code and I am sad. #1, Ready() then would be waiting until service is running?

Thoughts @stevesg @pstibrany ?

I like the idea of implementing WaitReady in terms of the waiting for the service to be running, seems like the service starting/running model fits nicely onto settling/broadcasting. Also means we can remove the readyc channel, which is nice.

Makes sense to me. I was just going to ask why do we still need readyc channel, good that you suggest removing it.

Love this idea as well!

Signed-off-by: Steve Simpson <[email protected]>

pracucci

LGTM (modulo a couple of nits) as 1st refactoring step.

pkg/alertmanager/alertmanager.go

pkg/alertmanager/state_replication_test.go

Signed-off-by: Steve Simpson <[email protected]>

pstibrany · 2021-03-09T19:47:12Z

pkg/alertmanager/alertmanager.go

 	am.Stop()
+
+	if service, ok := am.state.(services.Service); ok {
+		if err := service.AwaitTerminated(context.Background()); err != nil {


Shouldn’t we accept a context to StopAndWait method, and use that for waiting? (It has “wait” in the name, so it would make sense to me) In such case, we should also return error.

As much as I love putting context everywhere (getting good at it), I think it's better to address this in a separate PR, as this is a pre-existing problem with the code (e.g. the other Stop() functions all need to be updated, and the calling code).

pstibrany · 2021-03-09T19:50:20Z

pkg/alertmanager/state_replication.go

 }

-func (s *state) handleBroadcasts() {
+func (s *state) running(ctx context.Context) error {


Makes sense to me. I was just going to ask why do we still need readyc channel, good that you suggest removing it.

gotjosh

LGTM, Way better than the previous design!

pkg/alertmanager/alertmanager.go

gotjosh · 2021-03-10T09:43:42Z

pkg/alertmanager/state_replication.go

 }

-func (s *state) handleBroadcasts() {
+func (s *state) running(ctx context.Context) error {


Love this idea as well!

Signed-off-by: Steve Simpson <[email protected]>

…xproject#3930) * Use BasicService in state_replication.go for replication loop. Signed-off-by: Steve Simpson <[email protected]> * Review comments. Signed-off-by: Steve Simpson <[email protected]> * Review comments. Signed-off-by: Steve Simpson <[email protected]> * Review comments. Signed-off-by: Steve Simpson <[email protected]>

stevesg commented Mar 8, 2021

View reviewed changes

Use BasicService in state_replication.go for replication loop.

2c65d7a

Signed-off-by: Steve Simpson <[email protected]>

stevesg force-pushed the am-state-service branch from a3e86d2 to 2c65d7a Compare March 9, 2021 10:48

pull-request-size bot added the size/M label Mar 9, 2021

pracucci reviewed Mar 9, 2021

View reviewed changes

Review comments.

9400e12

Signed-off-by: Steve Simpson <[email protected]>

pracucci approved these changes Mar 9, 2021

View reviewed changes

pkg/alertmanager/alertmanager.go Outdated Show resolved Hide resolved

pkg/alertmanager/state_replication_test.go Show resolved Hide resolved

pkg/alertmanager/state_replication_test.go Outdated Show resolved Hide resolved

Review comments.

8cac29b

Signed-off-by: Steve Simpson <[email protected]>

pstibrany reviewed Mar 9, 2021

View reviewed changes

pstibrany approved these changes Mar 9, 2021

View reviewed changes

gotjosh approved these changes Mar 10, 2021

View reviewed changes

Review comments.

0d300f1

Signed-off-by: Steve Simpson <[email protected]>

stevesg force-pushed the am-state-service branch from a19b694 to 0d300f1 Compare March 10, 2021 10:30

pracucci merged commit b8d4761 into cortexproject:master Mar 10, 2021

stevesg mentioned this pull request Mar 11, 2021

Remove channel in favour of using Service.AwaitReady. #3940

Merged

3 tasks

	if service, ok := am.state.(*state); ok {
	if service, ok := am.state.(services.Service); ok {

Use BasicService in state_replication.go for replication loop. #3930

Use BasicService in state_replication.go for replication loop. #3930

Uh oh!

Conversation

stevesg commented Mar 8, 2021

Uh oh!

stevesg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevesg Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gotjosh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stevesg Mar 9, 2021 •

edited

Loading