Skip to content
This repository has been archived by the owner on Oct 23, 2024. It is now read-only.

Refactor MarathonScheduler.newTask #937

Closed
ConnorDoyle opened this issue Jan 5, 2015 · 6 comments
Closed

Refactor MarathonScheduler.newTask #937

ConnorDoyle opened this issue Jan 5, 2015 · 6 comments
Assignees
Milestone

Comments

@ConnorDoyle
Copy link
Contributor

There is a long-standing TODO in the in-line comments to fix the return value of this method. It currently returns an anonymous tuple wrapped in an option.

@hekaldama
Copy link
Contributor

The TODO comment was removed in #936 (646e0fd#diff-f4e1dabbf63bd911572e1049fad204f7L292) but the optimization / cleanup remains.

@hekaldama
Copy link
Contributor

@ConnorDoyle I found https://github.com/mesosphere/marathon/blob/master/src/main/scala/mesosphere/marathon/MarathonSchedulerActor.scala#L500-L516 as well. Does that need to be removed? Doesn't seem like anything is calling it, but not sure if I am understanding.

@kolloch kolloch modified the milestones: 0.8.3, 0.9.0, 0.10.0 May 18, 2015
@kolloch
Copy link
Contributor

kolloch commented May 18, 2015

TODO has been removed.

@ConnorDoyle
Copy link
Contributor Author

So, closing this signifies that the problem won't be fixed?

@kolloch kolloch self-assigned this May 19, 2015
@kolloch
Copy link
Contributor

kolloch commented May 19, 2015

I just noticed that the method was gone so that the issue text didn't make sense anymore. That was a mistake. Sorry. We are a bit overwhelmed with the long issue list and tried to remove the issues that are already done. Thanks for getting back to us about that.

I created a PR for this. I would appreciate your review since you brought this issue up.

Generally, I think it is not helpful creating issues for internal style issues which are that easily fixed without following up very soon. Otherwise our issues list will grow very long and that results in accumulated overhead in planning.

But I know that one can easily disagree about this.

@kolloch kolloch reopened this May 19, 2015
@ConnorDoyle
Copy link
Contributor Author

Thanks Peter, reviewing now.

ConnorDoyle added a commit that referenced this issue May 19, 2015
Fixes #937 - TaskFactory.newTask should return an Option of a case class
@d2iq-archive d2iq-archive locked and limited conversation to collaborators Mar 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants