Skip to content
This repository has been archived by the owner on Sep 26, 2022. It is now read-only.

sched: Add k8s scheduler support #72

Closed
wants to merge 1 commit into from

Conversation

gaocegege
Copy link
Contributor

No description provided.

Copy link
Contributor

@andreikeis andreikeis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few style things:

  • we do not use relative imports (sometime we do use relative import as in "from . import x" but for "from ... import x" - rather from treadmill.utils import x.

we do not import classes, just packages.

from treadmill.utils import X # bad
from treadmill import utils # good

utils.X()

...

this is rather big and ambitious change. can you summarize the intent separate from code - either in the comment or detailed commit message (apologies if i missed it).

if the two commits can be squashed into one, i think it will help and make review easier.

I am vacation right now, so my replies are rather sporadic. I will try and make a better / substantial review once I am back next week.

Thanks,
Andrei

def __init__(self, zkclient, cellname, events_dir=None):
def __init__(self, zkclient, cellname,
scheduler_vendor='native', config=None, events_dir=None):
if scheduler_vendor == 'k8s':

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have multiple vendors, then this should be an Enum

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless, it shoudl be a global variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is a good suggestion, I will update soon.

@gaocegege
Copy link
Contributor Author

Hi, all.

I will update the code soon to fix:

  • we do not use relative imports (sometime we do use relative import as in "from . import x" but for "from ... import x" - rather from treadmill.utils import x.

  • we do not import classes, just packages.

  • If we have multiple vendors, then this should be an Enum.

And I will write the details about this PR in a few days and post in this issue :)

Thanks for your review.

Signed-off-by: Ce Gao <ce.gao@outlook.com>
@gaocegege gaocegege changed the title sched: Add k8s scheduler support WIP: sched: Add k8s scheduler support Aug 6, 2017
@gaocegege gaocegege changed the title WIP: sched: Add k8s scheduler support sched: Add k8s scheduler support Sep 20, 2017
@gaocegege
Copy link
Contributor Author

Closing it since it is stale

@gaocegege gaocegege closed this Jul 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants