Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exposed YARN node address in the ProtoBuf and Python API #39

Merged
merged 1 commit into from Jul 16, 2018

Conversation

Projects
None yet
2 participants
@superbobry
Copy link
Contributor

commented Jul 14, 2018

Closes #38

I thought about adding a NodeId message to the ProtoBuf, but a str is simpler and seems to be sufficient for now, wdyt?

@jcrist
Copy link
Owner

left a comment

Overall this seems fine to me.

start_time : datetime
The start time, None if container has not started.
finish_time : datetime
The finish time, None if container has not finished.
"""
__slots__ = ('service_name', 'instance', '_state', 'yarn_container_id',
'start_time', 'finish_time')
'yarn_node_address', 'start_time', 'finish_time')
_params = ('service_name', 'instance', 'state', 'yarn_container_id',
'start_time', 'finish_time')

This comment has been minimized.

Copy link
@jcrist

jcrist Jul 15, 2018

Owner

Need to also add it to _params

This comment has been minimized.

Copy link
@superbobry

superbobry Jul 16, 2018

Author Contributor

How about removing _params here? _params == __slots__ so _get_params would behave as expected.

This comment has been minimized.

Copy link
@jcrist

jcrist Jul 16, 2018

Owner

_params == __slots__ so _get_params

There's a difference between '_state' vs state'.

@@ -348,7 +348,8 @@ def test_container():

kwargs = dict(service_name="foo",
instance=0,
yarn_container_id='container_1528138529205_0038_01_000001')
yarn_container_id='container_1528138529205_0038_01_000001'
yarn_node_address='localhost:14420')

This comment has been minimized.

Copy link
@jcrist

jcrist Jul 15, 2018

Owner

you're missing a comma above ^^, this is a syntax error.

This comment has been minimized.

Copy link
@superbobry

superbobry Jul 16, 2018

Author Contributor

Ouch, fixed.

@superbobry superbobry force-pushed the criteo-forks:yarn-node-address branch from 71d1957 to 7055a8f Jul 16, 2018

@jcrist

This comment has been minimized.

Copy link
Owner

commented Jul 16, 2018

This looks good to me. Thanks!

@jcrist jcrist merged commit 9d05ad5 into jcrist:master Jul 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.