Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

Updated edxapp Elasticsearch config #3887

Merged
merged 1 commit into from May 31, 2017

Conversation

clintonb
Copy link
Contributor

@clintonb clintonb commented May 26, 2017

The urllib3 library (used by the Elasticsearch Python library) expects ports to be integers, not strings, when passed as kwargs. Thus, we set the port to be an integer in Ansible.

@jibsheet
Copy link
Contributor

@MichaelRoytman is working in this area, I'll let him review/pull into another branch/etc

@clintonb
Copy link
Contributor Author

Thanks for the feedback. I will merge this once I verify it works as expected. My Docker image is still building.

@clintonb
Copy link
Contributor Author

@clintonb
Copy link
Contributor Author

I am going to use a different method of specifying the host and port to workaround this issue.

@clintonb clintonb force-pushed the clintonb/correct-edxapp-elasticsearch-port branch from ec00702 to e40c6de Compare May 26, 2017 17:36
@clintonb clintonb changed the title Rendering Elasticsearch port as integer for edxapp Updated edxapp Elasticsearch config May 26, 2017
@clintonb
Copy link
Contributor Author

@MichaelRoytman please take another look. I have confirmed this solves the connection issues for Docker Devstack. Indexing is still broken, but that's another issue altogether that will be fixed by https://github.com/edx/devstack/issues/40.

@jibsheet
Copy link
Contributor

jibsheet commented May 26, 2017

Fascinating - we have the s/'9200'/9200/ change in the secure repo as part of Michael's tests and I thought it was working (without switching to the full url style).

The elasticsearch init that we use in edx-search is almost too flexible about arguments...

@clintonb
Copy link
Contributor Author

@MichaelRoytman please review again.

@jibsheet
Copy link
Contributor

@clintonb So, Michael ran into "9200" vs 9200 earlier and we were fixing it by overriding with something like this

EDXAPP_SEARCH_HOST: 'search-host-name'
EDXAPP_SEARCH_PORT: 443
EDXAPP_SEARCH_USE_SSL: true

EDXAPP_ELASTIC_SEARCH_CONFIG:
  - host: "{{ EDXAPP_SEARCH_HOST }}"
    port: "{{ EDXAPP_SEARCH_PORT }}"
    use_ssl: "{{ EDXAPP_SEARCH_USE_SSL }}"

Which connects and works for us?

@MichaelRoytman
Copy link
Contributor

@clintonb If removing the quote around the port and keeping the dictionary works (Kevin's comment above), what does converting the dict to a list do? Or what is the advantage?

@clintonb
Copy link
Contributor Author

Do your changes work without SSL (e.g. port 9200)? Simply converting from "9200" to 9200 did not work for me.

The major change here is passing a complete URL instead of a dictionary. The library itself knows how to parse URLs: https://github.com/elastic/elasticsearch-py/blob/master/elasticsearch/client/__init__.py#L18.

@clintonb
Copy link
Contributor Author

I'm pleased to announce I was wrong! Removing the quotes from the port number resolve the problem. Clearly I tested with the wrong image/laptop. Updating the branch now.

@clintonb clintonb force-pushed the clintonb/correct-edxapp-elasticsearch-port branch 2 times, most recently from 39c09ae to b145fda Compare May 31, 2017 21:16
The urllib3 library (used by the Elasticsearch Python library) expects ports to be integers, not strings, when passed as kwargs. Thus, we set the port to be an integer in Ansible.
@clintonb clintonb merged commit ac48fba into master May 31, 2017
@clintonb clintonb deleted the clintonb/correct-edxapp-elasticsearch-port branch May 31, 2017 22:03
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.

None yet

3 participants