scheduler improvements and API tweaks #286

jdef · 2017-04-16T12:40:21Z

httpsched transitions to disconnected upon errors generated by the Decoder
demo scheduler properly handles Mesos master failover
httpcli: deprecate ProtocolError and Err variables in favor of APIError
minor doc tweaks

coveralls · 2017-04-16T12:45:47Z

Coverage increased (+0.01%) to 36.489% when pulling 70084cf on jdef_scheduler_improvements into 43f0fa5 on master.

coveralls · 2017-04-16T14:12:03Z

Coverage increased (+0.02%) to 36.496% when pulling 299e0ef on jdef_scheduler_improvements into 43f0fa5 on master.

coveralls · 2017-04-16T14:45:02Z

Coverage decreased (-0.03%) to 36.442% when pulling e98a3fb on jdef_scheduler_improvements into 43f0fa5 on master.

jdef · 2017-04-16T14:37:30Z

api/v1/cmd/example-scheduler/app/app.go

+		frameworkIDStore = NewInMemoryIDStore()
+		controlContext   = &controller.ContextAdapter{
+			DoneFunc: func() bool { return state.done },
+			// not called concurrently w/ subscription events, don't worry about using the atomic


bad comment

jdef · 2017-04-16T14:43:31Z

api/v1/lib/extras/scheduler/controller/controller.go

@@ -131,13 +131,19 @@ func (ca *ContextAdapter) Error(err error) {
 	}
 }

+type SchedulerError string


this is a pretty bad name. we can do better

jdef · 2017-04-16T15:05:16Z

api/v1/cmd/example-scheduler/app/app.go

+				frameworkID := e.GetSubscribed().GetFrameworkID().GetValue()
+				if state.frameworkID == "" || state.frameworkID != frameworkID {
+					if state.frameworkID != "" && state.frameworkID != frameworkID && state.config.checkpoint {
+						state.done = true // TODO(jdef) not goroutine safe


I wonder if these are really panic worthy conditions instead

coveralls · 2017-04-16T18:02:36Z

Coverage increased (+0.04%) to 36.514% when pulling 7c4c8c7 on jdef_scheduler_improvements into 43f0fa5 on master.

jdef · 2017-04-16T21:11:44Z

api/v1/lib/scheduler/calls/caller.go

@@ -85,6 +101,19 @@ func FrameworkCaller(frameworkID string) Decorator {
 	}
 }

+// DynamicFrameworkCaller generates and returns a Decorator that applies the given frameworkID to all calls (except SUBSCRIBE).
+func DynamicFrameworkCaller(frameworkID func() string) Decorator {


still dislike this name. maybe SubscribedCaller instead?

jdef · 2017-04-16T21:11:44Z

api/v1/lib/scheduler/calls/caller.go

@@ -85,6 +101,19 @@ func FrameworkCaller(frameworkID string) Decorator {
 	}
 }

+// DynamicFrameworkCaller generates and returns a Decorator that applies the given frameworkID to all calls (except SUBSCRIBE).
+func DynamicFrameworkCaller(frameworkID func() string) Decorator {


still dislike this name. maybe SubscribedCaller instead?

coveralls · 2017-04-17T00:10:35Z

Coverage decreased (-0.002%) to 36.475% when pulling 60818df on jdef_scheduler_improvements into 43f0fa5 on master.

jdef · 2017-04-17T00:19:25Z

api/v1/cmd/example-scheduler/app/app.go

-					state.frameworkID = e.GetSubscribed().GetFrameworkID().GetValue()
+				frameworkID := e.GetSubscribed().GetFrameworkID().GetValue()
+				if state.frameworkID == "" || state.frameworkID != frameworkID {
+					if state.frameworkID != "" && state.frameworkID != frameworkID && state.config.checkpoint {


not required to be nested, pull outside of containing if

jdef · 2017-04-17T00:19:54Z

api/v1/cmd/example-scheduler/app/config.go

@@ -84,6 +88,7 @@ func NewConfig() Config {
 		url:              env("MESOS_MASTER_HTTP", "http://:5050/api/v1/scheduler"),
 		codec:            codec{Codec: &encoding.ProtobufCodec},
 		timeout:          envDuration("MESOS_CONNECT_TIMEOUT", "20s"),
+		failoverTimeout:  envDuration("FRAMEWORK_FAILOVER_TIMEOUT", "1000h"),


SCHEDULER_FAILOVER_TIMEOUT

jdef · 2017-04-17T00:22:45Z

api/v1/lib/extras/scheduler/controller/controller.go

@@ -78,7 +76,9 @@ func (_ *controllerImpl) Run(config Config) (lastErr error) {
 	for !config.Context.Done() {
 		frameworkID := config.Context.FrameworkID()
 		if config.Framework.GetFailoverTimeout() > 0 && frameworkID != "" {
-			subscribe.Subscribe.FrameworkInfo.ID = &mesos.FrameworkID{Value: frameworkID}
+			frameworkProto := &mesos.FrameworkID{Value: frameworkID}
+			subscribe.Subscribe.FrameworkInfo.ID = frameworkProto


would be nice to consolidate these lines and say something like subscribe.WithFrameworkID(frameworkID)

coveralls · 2017-04-17T01:13:19Z

Coverage decreased (-0.07%) to 36.405% when pulling a8ba6d2 on jdef_scheduler_improvements into 43f0fa5 on master.

jdef · 2017-04-17T01:15:50Z

api/v1/lib/httpcli/http.go

 	})
 )

 // ProtocolError is a generic error type returned for expected status codes
 // received from Mesos.
+// Deprecated: no longer used in favor of APIError.


Might be better to just break the API now and remove this, otherwise downstream consumers will wonder why they're not seeing ProtocolError's anymore. Better to break the build and force change.

Maybe tackle this in a follow-up PR? This one is getting a bit long already.

coveralls · 2017-04-17T12:07:24Z

Coverage increased (+0.04%) to 36.521% when pulling 3db5f2c on jdef_scheduler_improvements into 43f0fa5 on master.

coveralls · 2017-04-17T13:37:43Z

Coverage increased (+0.01%) to 36.487% when pulling 17a0ad3 on jdef_scheduler_improvements into 43f0fa5 on master.

jdef · 2017-04-17T14:01:01Z

upon the scheduler receiving an ERROR event, it should probably re-try subscription. unless Mesos terminates the current subscription connection after sending an ERROR (which is not in the contract) then we're still in trouble because the httpsched state doesn't know anything about these ERROR messages so it will stay "connected" (and reject SUBSCRIBE attempts)

coveralls · 2017-04-17T14:51:31Z

Coverage decreased (-0.3%) to 36.195% when pulling a4cc120 on jdef_scheduler_improvements into 43f0fa5 on master.

coveralls · 2017-04-17T14:51:31Z

Coverage increased (+0.03%) to 36.504% when pulling a4cc120 on jdef_scheduler_improvements into 43f0fa5 on master.

coveralls · 2017-04-17T16:33:52Z

Coverage increased (+0.04%) to 36.519% when pulling 99f3370 on jdef_scheduler_improvements into 43f0fa5 on master.

jdef · 2017-04-17T16:45:16Z

This PR is massive enough and I'm mostly happy (enough) with it. Will keep it open for a few days for comments and merge if there are no complaints.

jdef · 2017-04-17T19:18:44Z

ideally there would be tests shipping with these changes

…ecting disconnect

…BSCRIBE

…zed clusters where all agents have GPU resources

…ler instead

…rFunc processes responses instead of status codes

jdef · 2017-04-24T18:13:27Z

fixes #277

coveralls · 2017-04-24T18:29:24Z

Coverage decreased (-0.01%) to 36.446% when pulling 7fd41ca on jdef_scheduler_improvements into c00d0c1 on master.

jdef · 2017-04-24T18:36:29Z

filed ticket to track tech debt for better unit tests in httpcli and httpsched: #291

jdef added the WIP label Apr 16, 2017

jdef commented Apr 16, 2017

View reviewed changes

jdef commented Apr 17, 2017

View reviewed changes

jdef added PTAL and removed WIP labels Apr 17, 2017

James DeFelice added 9 commits April 24, 2017 17:55

update documentation; introduce SchedulerError

3f2f407

improved failover for non-checkpointing scheduler

20c7a03

additional debugging; restore original internal caller state upon det…

0806021

…ecting disconnect

v1 scheduler API re-registration requires frameworkID **twice** in SU…

001040f

…BSCRIBE

undo redundant tracking of old state caller, we do not need it

b9c37ac

httpcli: introduce APIError, deprecate ProtocolError and Err* variables

479bbf2

add gpuClusterCompat flag to permit running the framework on speciali…

88d6111

…zed clusters where all agents have GPU resources

avoid breaking the FrameworkCaller API, introduce DynamicFrameworkCal…

ae60d1f

…ler instead

address review feedback and make state.done goroutine-safe

6efae19

James DeFelice added 6 commits April 24, 2017 17:55

s/DynamicFrameworkCaller/SubscribedCaller/

d64ccfa

address more review feedback (code cleanup)

85a8841

httpcli: move APIError (and friends) to apierrors package; ErrorMappe…

18db7e1

…rFunc processes responses instead of status codes

more error API cleanup

bd7142a

transition to disconnected upon scheduler ERROR events

5df42ce

introduce DisconnectionDetector and CodeIndicatingSubscriptionLoss

7fd41ca

jdef force-pushed the jdef_scheduler_improvements branch from 99f3370 to 7fd41ca Compare April 24, 2017 18:24

jdef merged commit d09f663 into master Apr 24, 2017

jdef deleted the jdef_scheduler_improvements branch April 24, 2017 18:37

jdef removed the PTAL label Apr 24, 2017

jdef mentioned this pull request Apr 24, 2017

next: document or improve handling of master disconnections #277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler improvements and API tweaks #286

scheduler improvements and API tweaks #286

jdef commented Apr 16, 2017 •

edited

Loading

coveralls commented Apr 16, 2017 •

edited

Loading

coveralls commented Apr 16, 2017 •

edited

Loading

coveralls commented Apr 16, 2017 •

edited

Loading

jdef Apr 16, 2017

jdef Apr 16, 2017

jdef Apr 16, 2017

coveralls commented Apr 16, 2017 •

edited

Loading

jdef Apr 16, 2017

jdef Apr 16, 2017

coveralls commented Apr 17, 2017 •

edited

Loading

jdef Apr 17, 2017

jdef Apr 17, 2017

jdef Apr 17, 2017

coveralls commented Apr 17, 2017 •

edited

Loading

jdef Apr 17, 2017

coveralls commented Apr 17, 2017 •

edited

Loading

coveralls commented Apr 17, 2017 •

edited

Loading

jdef commented Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017 •

edited

Loading

coveralls commented Apr 17, 2017 •

edited

Loading

jdef commented Apr 17, 2017

jdef commented Apr 17, 2017

jdef commented Apr 24, 2017

coveralls commented Apr 24, 2017 •

edited

Loading

jdef commented Apr 24, 2017

scheduler improvements and API tweaks #286

scheduler improvements and API tweaks #286

Conversation

jdef commented Apr 16, 2017 • edited Loading

coveralls commented Apr 16, 2017 • edited Loading

coveralls commented Apr 16, 2017 • edited Loading

coveralls commented Apr 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Apr 17, 2017 • edited Loading

coveralls commented Apr 17, 2017 • edited Loading

jdef commented Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017 • edited Loading

coveralls commented Apr 17, 2017 • edited Loading

jdef commented Apr 17, 2017

jdef commented Apr 17, 2017

jdef commented Apr 24, 2017

coveralls commented Apr 24, 2017 • edited Loading

jdef commented Apr 24, 2017

jdef commented Apr 16, 2017 •

edited

Loading

coveralls commented Apr 16, 2017 •

edited

Loading

coveralls commented Apr 16, 2017 •

edited

Loading

coveralls commented Apr 16, 2017 •

edited

Loading

coveralls commented Apr 16, 2017 •

edited

Loading

coveralls commented Apr 17, 2017 •

edited

Loading

coveralls commented Apr 17, 2017 •

edited

Loading

coveralls commented Apr 17, 2017 •

edited

Loading

coveralls commented Apr 17, 2017 •

edited

Loading

coveralls commented Apr 17, 2017 •

edited

Loading

coveralls commented Apr 17, 2017 •

edited

Loading

coveralls commented Apr 24, 2017 •

edited

Loading