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

Fix json body request apis #78

Merged
merged 3 commits into from
Jan 11, 2020
Merged

Conversation

dimon222
Copy link
Collaborator

Fix #75

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.

I applied this change to my env and it works great. Nice catch!

When I went to apply the change, I noticed a number of other requests in which dictionary data was conveyed in data= rather than json=. I don't really use any others, but wondering if those should be changed as well?

@dimon222
Copy link
Collaborator Author

dimon222 commented Jan 11, 2020

Its highly possible but I don't have necessary configured cluster to test them sadly.

@kevin-bates
Copy link
Member

I think we should change them all.

Looking at the requests package, we'll get the appropriate encoding if json= is used. Whereas it looks like we'll get form encoding using data=. So, even though the header contains Content-Type: application/json, the actual encoding of the body doesn't match the header and more sensitive libraries get heartburn.

In my attached test code, I was passing in the actual json when submitting the application. This was associated with a data= in our package. I believe that because this was properly encoded, submission works. However, if I pass in the dictionary (prior to its conversion to json), submission fails with a bad request exception.

Response finished with status: 400. Details: {"RemoteException":{"exception":"BadRequestException","message":"java.lang.Exception: Could not parse application id ","javaClassName":"org.apache.hadoop.yarn.webapp.BadRequestException"}}

Once I change cluster_submit_application() to also use json= and pass the dictionary data, then submission also works.

Since we document the data parameter as "dict data" and given that all methods that hardcode their data payloads (like kill, change-queue, change-priority, etc.) are actually passing dictionaries, then I believe the right thing is to change all data= to json=.

@dimon222 dimon222 changed the title Fix cluster_application_kill body Fix json body request apis Jan 11, 2020
@dimon222
Copy link
Collaborator Author

@kevin-bates alright, pushed the commit and renamed the PR.

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.

Thanks Dmitry! Given the evidence of the methods that are used, I think this is the right approach.

@kevin-bates kevin-bates merged commit 2ea5f85 into gateway-experiments:master Jan 11, 2020
@dimon222 dimon222 deleted the patch-1 branch January 11, 2020 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ResourceManager.cluster_application_kill() is not working
2 participants