Refactor and move event handling code into the cluster package by nishanttotla · Pull Request #2752 · docker-archive/classicswarm

nishanttotla · 2017-06-29T23:24:01Z

In this PR

Remove watchQueue as a cluster object, and package it inside the APIEventHandler
Move events code to the cluster package
Refactor functions and structs with more descriptive names
Remove redundant code

Addresses #2749
Follows up #2747

nishanttotla · 2017-07-05T17:18:56Z

Ping @allencloud can you take a look?

allencloud · 2017-07-06T01:09:16Z

OK, I will today. @nishanttotla

Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

This is to eliminate some confusion around how events are handled in Swarm. Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

function Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

alexmavr

Implementation looks good, just a couple comments on code structure. I ran a couple smoke-tests locally and this PR does not seem to be causing any behavioral changes on first glance.

alexmavr · 2017-07-21T00:23:31Z

cluster/events.go

+
+// EventHandler is exported
+type EventHandler interface {
+	Handle(*Event) error


It looks like at this point all implementations of this method always return nil instead of an error. How about making this not return anything?

Seems like you're right. I can't recall why it was this way in the past, because generally event related errors don't cause things to fail, but perhaps in the future we might want to log those errors. What do you think?

Personally, I like to avoid returning an error when one is never expected to occur, but I don't have a strong preference here.

alexmavr · 2017-07-21T00:29:05Z

cluster/event_map.go

+// CloseWatchQueues unregisters all API event handlers (the ones with
+// watch queues) and closes the respective queues. This should be
+// called when the manager shuts down
+func (eh *ClusterEventHandlers) CloseWatchQueues() {


This could also be structured as a generic Cleanup method. Then, we can switch on the type of the handler below and perform any sort of type-specific cleanup (such as closing the watch queue for APIEventHandlers).

This is probably a vain effort though, as the other handlers don't require any cleanup, and I don't think we expect any new types of event handlers with the current architecture

I agree that this effort isn't very useful at this time, and we will not have more types of event handlers. Besides, it won't be hard to change it in the future if necessary.

alexmavr · 2017-07-21T00:31:43Z

cluster/event_map.go

+}
+
+// HandleAll callbacks for the events
+func (eh *ClusterEventHandlers) HandleAll(e *Event) {


Why not name this Handle so that the ClusterEventHandlers also satisfies the EventHandler interface?

I called it HandleAll for more clarity about what it does, and it didn't seem like we got anything by making it satisfy the interface.

The benefit to the rename is that we don't have to implement a Cluster.Handle method at all, if we embed Cluster.ClusterEventHandlers

alexmavr · 2017-07-21T00:32:48Z

cluster/mesos/cluster.go

@@ -169,31 +167,31 @@ func NewCluster(scheduler *scheduler.Scheduler, TLSConfig *tls.Config, master st

 // Handle callbacks for the events
 func (c *Cluster) Handle(e *cluster.Event) error {


If the Cluster struct were to embed a clusterEventHandlers, and the HandleAll method were renamed to Handle, this method wrapper could be removed entirely, as well as the Register/Unregister methods below.

This comment also applies for the swarm cluster object as well

This sounds like a reasonable thing to do. I believe in addition to renaming HandleAll, we'll also need to change the interface to not have Handle return the error, as you said above.

alexmavr · 2017-07-21T00:33:46Z

cluster/mesos/cluster.go

+// CloseWatchQueues unregisters all API event handlers (the ones with
+// watch queues) and closes the respective queues. This should be
+// called when the manager shuts down
+func (c *Cluster) CloseWatchQueues() {


This method looks like a no-op, is there any reason why it can't be removed?

Oh, this is a mistake I think. It was supposed to call the CloseWatchQueues function of ClusterEventHandlers.

Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

nishanttotla · 2017-07-21T20:48:37Z

@alexmavr Addressed your comments, and embedded ClusterEventsHandler inside the Cluster struct.

I've kept the error returned by the handler as of now, because I want to investigate if it makes sense to error out in some places (this I plan to do as a follow-up)

alexmavr

LGTM. Tests still pass with the new changes

alexmavr · 2017-07-21T20:51:49Z

cluster/mesos/cluster.go

+		refuseTimeout:       defaultRefuseTimeout,
+	}
+
+	cluster.InitClusterEventHandlers()


That method change wasn't really required, you can still initialize an embedded struct, such as:

cluster := &Cluster{ ClusterEventHandlers: cluster.NewClusterEventHandlers(), }

Oh yeah, I had changed it earlier for some reason, but forgot to fix it later. I'll change it.

…side the Cluster object Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

nishanttotla added the area/event label Jun 29, 2017

nishanttotla added this to the 1.3.0 milestone Jun 29, 2017

nishanttotla force-pushed the events-refactor-watchdog branch 2 times, most recently from dbfd98b to eb42260 Compare June 30, 2017 00:30

nishanttotla mentioned this pull request Jun 30, 2017

Revamp event handling #2749

Closed

3 tasks

nishanttotla force-pushed the events-refactor-watchdog branch 2 times, most recently from 9249e16 to eeafb8d Compare June 30, 2017 17:22

nishanttotla changed the title ~~[WIP] Major events refactoring~~ Major events refactoring Jun 30, 2017

nishanttotla changed the title ~~Major events refactoring~~ Refactor and move event handling code into the cluster package Jun 30, 2017

nishanttotla added the kind/enhancement label Jun 30, 2017

nishanttotla force-pushed the events-refactor-watchdog branch 3 times, most recently from 3edec95 to 839ce5f Compare July 5, 2017 16:38

nishanttotla force-pushed the events-refactor-watchdog branch 4 times, most recently from a3c9c7d to 2342a32 Compare July 15, 2017 01:12

nishanttotla added 5 commits July 17, 2017 23:13

Move cluster watchQueue into eventsHandler

5a53a30

Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

Moving API event handler code to cluster package

eb9d3dc

Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

More descriptive names for event structs and files

3abe106

This is to eliminate some confusion around how events are handled in Swarm. Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

Make it possible to close API event watchqueues using a cluster level

d81c835

function Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

Refactoring event code and removing unused code

27e8938

Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

nishanttotla force-pushed the events-refactor-watchdog branch from 2342a32 to 9f062b6 Compare July 18, 2017 06:13

alexmavr approved these changes Jul 21, 2017

View reviewed changes

Moving listener count to APIEventHandler

f07e720

Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

nishanttotla force-pushed the events-refactor-watchdog branch 2 times, most recently from 9d05bfa to 4fcf4a1 Compare July 21, 2017 20:36

nishanttotla modified the milestones: 1.2.9, 1.3.0 Jul 21, 2017

alexmavr approved these changes Jul 21, 2017

View reviewed changes

Make ClusterEventHandlers satisfy EventHandler interface and embed in…

995e078

…side the Cluster object Signed-off-by: Nishant Totla <nishanttotla@gmail.com>

nishanttotla force-pushed the events-refactor-watchdog branch from 4fcf4a1 to 995e078 Compare July 21, 2017 23:03

nishanttotla merged commit d28fd0d into docker-archive:master Jul 21, 2017

nishanttotla deleted the events-refactor-watchdog branch July 21, 2017 23:47

		@@ -169,31 +167,31 @@ func NewCluster(scheduler scheduler.Scheduler, TLSConfig tls.Config, master st

		// Handle callbacks for the events
		func (c Cluster) Handle(e cluster.Event) error {

Conversation

nishanttotla commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nishanttotla commented Jul 5, 2017

Uh oh!

allencloud commented Jul 6, 2017

Uh oh!

alexmavr left a comment

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!

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!

nishanttotla Jul 21, 2017 • 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!

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!

nishanttotla commented Jul 21, 2017

Uh oh!

alexmavr left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nishanttotla commented Jun 29, 2017 •

edited

Loading

nishanttotla Jul 21, 2017 •

edited

Loading