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

ES: update dependencies #7408

Merged
merged 2 commits into from Aug 26, 2020
Merged

ES: update dependencies #7408

merged 2 commits into from Aug 26, 2020

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Aug 25, 2020

Getting ready to upgrade to the latest version

I read the whole commit history from es_dsl follow the changes, so hopefully didn't miss anything :D I still need to double check django_es_dsl and test this locally.

Closes #6309
ref #5620

Getting ready to upgrade to the latest version
Copy link
Member

@humitos humitos left a comment

Code changes look good to me!

I tested this locally and it failed when performing a search from the dashboard: http://community.dev.readthedocs.io/search/?q=sphinx&type=file

Also, making a search from the homepage did work but it didn't show any result when searching http://community.dev.readthedocs.io/search/?q=sphinx

Am I missing something to test this properly? I did upgrade my ES to 6.5.4

@stsewd
Copy link
Member Author

stsewd commented Aug 25, 2020

@humitos this isn't ready for review, I haven't tested it locally yet.

@stsewd stsewd added the PR: work in progress Pull request is not ready for full review label Aug 25, 2020
@stsewd
Copy link
Member Author

stsewd commented Aug 25, 2020

I tested this locally, is working. @humitos make sure you are installing the new deps in a clean environment, and also that search was working previously.

@stsewd stsewd requested review from ericholscher and a team Aug 25, 2020
@stsewd stsewd removed the PR: work in progress Pull request is not ready for full review label Aug 26, 2020
Copy link
Member

@ericholscher ericholscher left a comment

This looks like a good upgrade to do, but I think we probably want to think more about how we can simplify this code. It's pretty complex, and I'd love to make our search reindexing more solid and simple.

@@ -1,7 +1,7 @@
import logging

from django.conf import settings
from django_elasticsearch_dsl import DocType, Index, fields
from django_elasticsearch_dsl import Document, Index, fields
Copy link
Member

@ericholscher ericholscher Aug 26, 2020

Choose a reason for hiding this comment

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

I'm curious if we're getting much value from the django integration here. It might make sense to look into just using the elasticsearch_dsl directly. I think we've disabled most of the Django-specific integration, so that might make our code easier to manage and a lot more explicit.

Copy link
Member Author

@stsewd stsewd Aug 26, 2020

Choose a reason for hiding this comment

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

Yeah, I saw that too. The main feature of the django integration is that it syncs the ES index when the models change, but we don't make use of that. I'll make a note.

@stsewd
Copy link
Member Author

stsewd commented Aug 26, 2020

@ericholscher this update will change the response of the old API (docsearch/). But for the next deploy is going to be 2 weeks since the change for the new API. Is 2 weeks enough for browsers to pick the new API instead of the old one? Or should we hold off one more week before merging this?

@ericholscher
Copy link
Member

ericholscher commented Aug 26, 2020

@stsewd I think we are safe to deploy it now. I think a week is plenty of time 👍

@ericholscher
Copy link
Member

ericholscher commented Aug 26, 2020

What exactly is changing with the old API?

@stsewd
Copy link
Member Author

stsewd commented Aug 26, 2020

@stsewd stsewd merged commit 23fb804 into master Aug 26, 2020
2 checks passed
@stsewd stsewd deleted the update-es-dep branch Aug 26, 2020
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.

None yet

3 participants