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.

Sign up for GitHub

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

Jump to bottom

Add missing RM apis #48

Merged
merged 3 commits into from
Sep 24, 2019
Merged

Add missing RM apis #48

merged 3 commits into from
Sep 24, 2019

Conversation

dimon222
Copy link
Collaborator

@dimon222 dimon222 commented Sep 20, 2019 •
edited
Loading

This PR is to cover missing APIs for RM mentioned in #34 (comment)

Some gaps left still, not sure if it worth merging before filling those.

  • Tests are not written, I've added all the "TODO" tests that has to be still coded
  • Endpoints of Delegation token renewal/cancelation introduce new behavior which we didn't cover previously - its parsing token out of headers. Need to validate if its working.

It's only covering RM, not other components (that might still have some API off)

@dimon222 dimon222 mentioned this pull request Sep 20, 2019
Closed
@dimon222 dimon222 force-pushed the feature/add_missing_rm_apis branch 2 times, most recently from 671aeea to a72c72f Compare September 21, 2019 01:27
kevin-bates
kevin-bates reviewed Sep 23, 2019
View reviewed changes
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

Great stuff @dimon222!

Just had the one comment regarding headers.

yarn_api_client/base.py Show resolved Hide resolved
yarn_api_client/resource_manager.py Show resolved Hide resolved
@dimon222
Copy link
Collaborator Author

@kevin-bates i've applied specified corrections and added all the tests I was able to figure out.
There's a concern with cluster_scheduler_queue for which I couldn't finish test. I think the actual method implementation is incorrect, so I'm not able to replicate successful path at all. If there's an example of success, it would be useful. The rest - covered.

kevin-bates
kevin-bates approved these changes Sep 24, 2019
View reviewed changes
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @dimon222!

@kevin-bates
Copy link
Member

@dimon222 - could you please rebase your branch with master? I'll merge your PRs tomorrow - assuming there are no objections. Thanks.

@dimon222 dimon222 force-pushed the feature/add_missing_rm_apis branch 5 times, most recently from 15de9a4 to 8cfb72e Compare September 24, 2019 00:39
@kevin-bates kevin-bates merged commit 12a2fd6 into gateway-experiments:master Sep 24, 2019
@kevin-bates
Copy link
Member

Thanks @dimon222!

@kevin-bates kevin-bates mentioned this pull request Sep 24, 2019
Merged
@dimon222 dimon222 deleted the feature/add_missing_rm_apis branch September 24, 2019 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevin-bates kevin-bates kevin-bates approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dimon222 @kevin-bates