HomeMesosphereNo notifications. 4 unresolved issues.

MARATHON-1135 Replace TaskStatusEmitter usage with EventBus
ClosedAll Users

Authored by meln1k on Sep 6 2017, 6:35 PM.

Diff Detail

Repository
rMARATHON marathon
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meln1k created this revision.Sep 6 2017, 6:35 PM
jenkins requested changes to this revision.Sep 6 2017, 6:36 PM
This revision now requires changes to proceed.Sep 6 2017, 6:36 PM
✗ Build of 4213 failed jenkins-public-marathon-phabricator-960.

Error message:

Stage Compile and Test failed.

(๑′°︿°๑)

ichernetsky accepted this revision.Sep 6 2017, 7:05 PM
ichernetsky added inline comments.
src/test/scala/mesosphere/marathon/core/flow/impl/OfferMatcherLaunchTokensActorTest.scala
19

Please remove this empty line.

jenkins accepted this revision.Sep 6 2017, 7:23 PM
This revision is now accepted and ready to land.Sep 6 2017, 7:23 PM
✔ Build of 4213 completed jenkins-public-marathon-phabricator-961.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

\\ ٩( ᐛ )و //

meln1k updated this revision to Diff 4221.Sep 7 2017, 2:13 PM
  • MARATHON-1135 fixes after the review
jenkins requested changes to this revision.Sep 7 2017, 2:15 PM
This revision now requires changes to proceed.Sep 7 2017, 2:15 PM
✗ Build of 4221 failed jenkins-public-marathon-phabricator-969.

Error message:

Stage Compile and Test failed.

(๑′°︿°๑)

✗ Build of 4221 failed jenkins-public-marathon-phabricator-970.

Error message:

Stage Compile and Test failed.

(๑′°︿°๑)

jenkins accepted this revision.Sep 7 2017, 3:26 PM
This revision is now accepted and ready to land.Sep 7 2017, 3:26 PM
✔ Build of 4221 completed jenkins-public-marathon-phabricator-971.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

\\ ٩( ᐛ )و //

kensipe accepted this revision.Sep 7 2017, 3:35 PM
meichstedt requested changes to this revision.Sep 7 2017, 6:57 PM
meichstedt added a subscriber: meichstedt.

Thanks for the fast results! As noted you can simply remove everything related to TaskStatusEmitterPublishStepImpl. InstanceChanges are already published via the PostToEventStreamStepImpl.

src/main/scala/mesosphere/marathon/core/task/update/impl/steps/TaskStatusEmitterPublishStepImpl.scala
0

This whole class and related wiring code can be removed. There is the PostToEventStreamStepImpl that handles publishing instance changes to the event stream. Sorry this wasn't clear from the JIRA :|

This revision now requires changes to proceed.Sep 7 2017, 6:57 PM
meln1k added inline comments.Sep 8 2017, 12:35 PM
src/main/scala/mesosphere/marathon/core/task/update/impl/steps/TaskStatusEmitterPublishStepImpl.scala
0

PostToEventStreamStepImpl publishes a little bit different events but I can inline the mentioned line in it.

meln1k updated this revision to Diff 4231.Sep 8 2017, 1:01 PM
  • MARATHON-1135 fixes after the review
jenkins requested changes to this revision.Sep 8 2017, 1:02 PM
This revision now requires changes to proceed.Sep 8 2017, 1:02 PM
✗ Build of 4231 failed jenkins-public-marathon-phabricator-980.

Error message:

Stage Package Docker Image, Debian and RedHat Packages failed.

(๑′°︿°๑)

meln1k updated this revision to Diff 4236.Sep 8 2017, 4:44 PM
  • MARATHON-1135 fixes after the review
jenkins requested changes to this revision.Sep 8 2017, 4:47 PM
This revision now requires changes to proceed.Sep 8 2017, 4:47 PM
✗ Build of 4236 failed jenkins-public-marathon-phabricator-987.

Error message:

Stage Compile and Test failed.

(๑′°︿°๑)

jenkins accepted this revision.Sep 8 2017, 6:26 PM
✔ Build of 4236 completed jenkins-public-marathon-phabricator-990.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

\\ ٩( ᐛ )و //

zen-dog resigned from this revision.Sep 8 2017, 6:28 PM
meichstedt requested changes to this revision.Sep 11 2017, 2:16 PM

Nearly there ;)

src/main/scala/mesosphere/marathon/core/flow/impl/OfferMatcherLaunchTokensActor.scala
32

This should subscribe to InstanceChanged instead (notice the d at the end). The PostToEventStreamStepImpl will publish all events contained in InstanceChange.events. These events are created by InstanceChangedEventsGenerator (the fact that InstanceHealthChanged is handled differently is a tech debt and there's a JIRA for it).

45

Using InstanceChanged, this would be

case InstanceChanged(_, _, _, _, instance) if instance.isRunning && instance.state.healthy.fold(true)(_ == true) =>
  offerMatcherManager.addLaunchTokens(1)
src/main/scala/mesosphere/marathon/core/task/update/impl/steps/PostToEventStreamStepImpl.scala
24 ↗(On Diff #4236)

Please remove this again – not needed, see other comments

meln1k added inline comments.Sep 11 2017, 3:12 PM
src/main/scala/mesosphere/marathon/core/flow/impl/OfferMatcherLaunchTokensActor.scala
32

Got it, thanks!

meln1k updated this revision to Diff 4251.Sep 11 2017, 4:50 PM
  • MARATHON-1135 fixes after the review
jenkins requested changes to this revision.Sep 11 2017, 4:58 PM
This revision now requires changes to proceed.Sep 11 2017, 4:58 PM

Error message:

Stage Compile and Test failed.

(๑′°︿°๑)

jenkins accepted this revision.Sep 11 2017, 5:48 PM
✔ Build of 4251 completed jenkins-public-marathon-phabricator-1001.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

\\ ٩( ᐛ )و //

meichstedt accepted this revision.Sep 12 2017, 11:42 AM

Thanks! I left two comments I'd like to have addressed, but they're only about deleting a few more lines.

src/test/scala/mesosphere/marathon/core/flow/impl/OfferMatcherLaunchTokensActorTest.scala
4

unused import

13–14

please remove this as well – you should then be able to remove the rx.lang.scala and the InstanceChange imports

This revision is now accepted and ready to land.Sep 12 2017, 11:42 AM
meln1k updated this revision to Diff 4256.Sep 12 2017, 2:01 PM
  • MARATHON-1135 fixes after the review
jenkins requested changes to this revision.Sep 12 2017, 2:01 PM
This revision now requires changes to proceed.Sep 12 2017, 2:01 PM
jenkins accepted this revision.Sep 12 2017, 2:16 PM
This revision is now accepted and ready to land.Sep 12 2017, 2:16 PM
✔ Build of 4256 completed jenkins-public-marathon-phabricator-1004.

You can create a DC/OS with your patched Marathon by creating a new pull
request with the following changes in buildinfo.json:

\\ ٩( ᐛ )و //

This revision was automatically updated to reflect the committed changes.
meichstedt updated JIRA issue(s): added 1 MARATHON_EE-1135; removed 1 MARATHON-1135.Sep 22 2017, 1:32 PM