-
Notifications
You must be signed in to change notification settings - Fork 48
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
The protocol scheme was added in the phase of finding the active RM. #67
The protocol scheme was added in the phase of finding the active RM. #67
Conversation
Can you confirm that you tried it on python 3.8.1? |
@dimon222 I faced with the issue (lacking of the protocol scheme) using Python 3.8.0 version. I've just updated Python version on 3.8.1 and run the unit tests:
Indeed, not all tests successfully completed on the 3.8.1 version. On the 3.7.6 Python version the situation is the same: `λ C:\temp\venv\hadoop-yarn-api-python-client\Scripts\python.exe --version Testing started at 11:42 PM ... DEBUG:yarn_api_client.application_master:Get configuration from hadoop conf dir Error DEBUG:requests_mock.adapter:GET https://example2:8022/cluster 200 Ran 85 tests in 0.199s FAILED (errors=1) Process finished with exit code 1 Assertion failed Assertion failed Assertion failed ` I used the following packs in Python environment: certifi 2019.11.28 |
Interesting, I'm able to run tests successfully on 3.8.0 UPD: UPD2: |
@dimon222 I think I found the interesting thing: Python 3.8.1 (tags/v3.8.1:1b293b6, Dec 18 2019, 22:39:24) [MSC v.1916 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from urllib.parse import urlparse
>>> urlparse('example.com:80')
ParseResult(scheme='example.com', netloc='', path='80', params='', query='', fragment='')
>>> Python 3.7.4 (default, Jul 9 2019, 00:06:43)
[GCC 6.3.0 20170516] on linux
>>> from urllib.parse import urlparse
>>> urlparse('example.com:80')
ParseResult(scheme='', netloc='', path='example.com:80', params='', query='', fragment='')
>>> It's due with the test "test_valid_request_with_parameters()": def get_client(self):
client = base.BaseYarnAPI()
client.service_uri = base.Uri('example.com:80') where base.Uri is class Uri(object):
def __init__(self, service_endpoint):
service_uri = urlparse(service_endpoint) |
I think I confused this thread with another issue altogether. My apology for this.
Thanks for helping, hopefully @lresende @kevin-bates can assist on reviewing this PR. |
You're welcome =) I'm glad I was able to help. |
@@ -103,7 +104,7 @@ def test_get_resource_endpoint(self): | |||
|
|||
endpoint = hadoop_conf.get_resource_manager_endpoint() | |||
|
|||
self.assertEqual('example.com:8022', endpoint) | |||
self.assertEqual('http://example.com:8022', endpoint) | |||
parse_mock.assert_called_with(hadoop_conf_path + 'yarn-site.xml', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these tests not failing without these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test changed because result value now will include schema (in same PR, scroll down a bit)
https://github.com/toidi/hadoop-yarn-api-python-client/blob/f88ac6966eff575b18563ef8829967546cf3cc3f/yarn_api_client/hadoop_conf.py#L43
Hello! This test is failing because |
I have added a PR that enables builds with the latest Python 3.8.x and I still cannot see any breaking build issues - see build results. Having said that, I will trust @dimon222 and/or @kevin-bates judgment here. Please let me know what do you guys think. |
@lresende test is written in assumption that it was working with this type of URL, but I guess it wasn't. It's highly possible that regression was introduced when active_rm check was changed to utilize requests library, as it's the requests library that looks for scheme. It might have been missed due to not many people directly utilizing read from XML config file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good to me. My only issue was the print()
statement, but that's just following the existing precedent. I have opened #73 to address this.
@lresende anything pending on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I faced with the issue "Python request: No connection adapters were found for..." when I passed yarn-site.xml file and called the constructor of the class ResourceManager without any arguments.
According to the Yarn documentation https://hadoop.apache.org/docs/current/hadoop-yarn/hadoop-yarn-site/ResourceManagerHA.html
the protocol scheme isn't stored in the values of parameters yarn.resourcemanager.webapp.address.rm-id & yarn.resourcemanager.webapp.https.address.rm-id