HomeMesosphereNo notifications. 4 unresolved issues.
  • Search
Differential D320

Reverted the `group` function to not use `transitiveGroupsById`.
ClosedAll Users
Actions

Authored by mpark on Dec 12 2016, 7:21 PM.

Details

Reviewers
aquamatthias
timcharper
• jasongilanfarr
Commits
rMARATHONbed46d122dd8: Reverted the `group` function to not use `transitiveGroupsById`.
rMARATHONe2a01e1da095: Reverted the `group` function to not use `transitiveGroupsById`.
rMARATHONc8b1742efb8b: Reverted the `group` function to not use `transitiveGroupsById`.
Summary

Reduced the instantiation of transitiveGroupsById by avoiding it in the group function.

Test Plan

sbt test

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.
mpark retitled this revision from to Reverted the `group` function to not use `transitiveGroupsById`..Dec 12 2016, 7:21 PM
mpark updated this object.
mpark edited the test plan for this revision. (Show Details)
mpark added reviewers: jeschkies, • jasongilanfarr, aquamatthias.
mpark added a subscriber: marathon-team.
aquamatthias edited reviewers, added: timcharper; removed: jeschkies.Dec 12 2016, 8:02 PM
timcharper accepted this revision.Dec 12 2016, 8:31 PM
timcharper added inline comments.
src/main/scala/mesosphere/marathon/state/Group.scala
47

I’m finding this code quite difficult to parse. Can we add some documentation to how the method works?

This revision is now accepted and ready to land.Dec 12 2016, 8:31 PM
timcharper added inline comments.Dec 12 2016, 9:07 PM
src/main/scala/mesosphere/marathon/state/Group.scala
47

how about this?

/**
  * Find and return the child group for the given path. If no match is found, then returns None
  */
def group(gid: PathId): Option[Group] = {
  if (id == gid) Some(this)
  else {
    val immediateChild = gid.restOf(id).root
    groupsById.find { case (_, group) => group.id.restOf(id).root == immediateChild }
      .flatMap { case (_, group) => group.group(gid) }
  }
}
• jasongilanfarr accepted this revision.Dec 12 2016, 9:51 PM

Please use tim's suggestion

timcharper updated this revision to Diff 1240.Dec 12 2016, 9:57 PM

Add docs. Clarify naming.

Closed by commit rMARATHONc8b1742efb8b: Reverted the `group` function to not use `transitiveGroupsById`. (authored by mpark, committed by timcharper). · Explain WhyDec 12 2016, 9:59 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

Diff 1241

View Options

src/main/scala/mesosphere/marathon/state/Group.scala

Show All 31 Lines
3232 .addAllDeprecatedApps(apps.map { case (_, app) => app.toProto })
3333 .addAllGroups(groupsById.map { case (_, group) => group.toProto })
3434 .addAllDependencies(dependencies.map(_.toString))
3535 .build()
3636 }
3737
3838 def app(appId: PathId): Option[AppDefinition] = transitiveAppsById.get(appId)
3939 def pod(podId: PathId): Option[PodDefinition] = transitivePodsById.get(podId)
40 def group(groupId: PathId): Option[Group] = transitiveGroupsById.get(groupId)
40
41 /**
42 * Find and return the child group for the given path. If no match is found, then returns None
43 */
44 def group(gid: PathId): Option[Group] = {
45 if (id == gid) Some(this)
46 else {
47 val immediateChild = gid.restOf(id).root
timcharperUnsubmitted
Not Done

I’m finding this code quite difficult to parse. Can we add some documentation to how the method works?

timcharperUnsubmitted
Not Done

how about this?

/**
  * Find and return the child group for the given path. If no match is found, then returns None
  */
def group(gid: PathId): Option[Group] = {
  if (id == gid) Some(this)
  else {
    val immediateChild = gid.restOf(id).root
    groupsById.find { case (_, group) => group.id.restOf(id).root == immediateChild }
      .flatMap { case (_, group) => group.group(gid) }
  }
}
48 groupsById.find { case (_, group) => group.id.restOf(id).root == immediateChild }
49 .flatMap { case (_, group) => group.group(gid) }
50 }
51 }
4152
4253 lazy val transitiveApps: Set[AppDefinition] = transitiveAppsById.values.toSet
4354 lazy val transitiveAppIds: Set[PathId] = transitiveAppsById.keySet
4455
4556 lazy val transitiveRunSpecsById: Map[PathId, RunSpec] = transitiveAppsById ++ transitivePodsById
4657 lazy val transitiveRunSpecs: Set[RunSpec] = transitiveRunSpecsById.values.toSet
4758
4859 lazy val transitiveGroupsById: Map[Group.GroupKey, Group] = {
▲ Show 20 Lines • Show All 102 Lines • Show Last 20 Lines