Skip to content

Upgrade search app to Elastic Search 5.4 #3373

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

Closed
wants to merge 22 commits into from
Closed

Upgrade search app to Elastic Search 5.4 #3373

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f291550
Upgrade search app to Elastic Search 5.4
ericholscher Dec 7, 2017
a922986
Update ES dep
ericholscher Dec 7, 2017
b7e1189
Integrate ext search
ericholscher Dec 7, 2017
848ee3c
Don't require ext search
ericholscher Dec 7, 2017
e5c42c0
Fix URL insertion
ericholscher Dec 7, 2017
4cf429f
Merge remote-tracking branch 'origin/master' into upgrade-es
ericholscher Dec 7, 2017
1291961
Fix lint
ericholscher Dec 11, 2017
a4208de
Fix this here too, for now :)
ericholscher Dec 11, 2017
ec992c3
Use proper URL insert
ericholscher Dec 12, 2017
8200055
Merge remote-tracking branch 'origin/master' into upgrade-es
ericholscher Dec 12, 2017
d0a8ce2
Use old append
ericholscher Dec 12, 2017
0c538f8
Leave out unrelated change
ericholscher Dec 12, 2017
75cf46f
Merge remote-tracking branch 'origin/master' into upgrade-es
ericholscher Dec 12, 2017
f2d0758
Default sections off and fix unicode on index
ericholscher Dec 12, 2017
ede0827
Merge remote-tracking branch 'origin/master' into upgrade-es
ericholscher Dec 14, 2017
87c1c9d
Run task via celery to do concurrency
ericholscher Dec 19, 2017
213a390
Lint reindex_elasticsearch
ericholscher Dec 19, 2017
9dafb8b
Merge remote-tracking branch 'origin/master' into upgrade-es
ericholscher Dec 19, 2017
07be69d
Lint utils
ericholscher Dec 19, 2017
dedf97f
fix up loggin
ericholscher Dec 19, 2017
24f9776
Make reindexing better
ericholscher Dec 20, 2017
ba8c675
Fix lint
ericholscher Dec 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions readthedocs/core/management/commands/reindex_elasticsearch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,15 @@ def handle(self, *args, **options):
"""Build/index all versions or a single project's version"""
project = options['project']

queryset = Version.objects.all()
queryset = Version.objects.filter(active=True)

if project:
queryset = queryset.filter(project__slug=project)
if not queryset.exists():
raise CommandError(
'No project with slug: {slug}'.format(slug=project))
log.info("Building all versions for %s", project)
elif getattr(settings, 'INDEX_ONLY_LATEST', True):
if getattr(settings, 'INDEX_ONLY_LATEST', True):
queryset = queryset.filter(slug=LATEST)

for version in queryset:
Expand Down
2 changes: 1 addition & 1 deletion readthedocs/restapi/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
url(r'index_search/',
search_views.index_search,
name='index_search'),
url(r'search/$', views.search_views.search, name='api_search'),
url(r'^search/$', views.search_views.search, name='api_search'),
url(r'search/project/$',
search_views.project_search,
name='api_project_search'),
Expand Down
5 changes: 2 additions & 3 deletions readthedocs/restapi/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,11 +141,10 @@ def index_search_request(version, page_list, commit, project_scale, page_scale,
'weight': page_scale,
})
for route in routes:
section_obj.bulk_index(section_index_list, parent=page_id,
routing=route)
section_obj.bulk_index(section_index_list, routing=route)

for route in routes:
page_obj.bulk_index(index_list, parent=project.slug, routing=route)
page_obj.bulk_index(index_list, routing=route)

if delete:
log.info("Deleting files not in commit: %s", commit)
Expand Down
2 changes: 1 addition & 1 deletion readthedocs/restapi/views/footer_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def get_version_compare_data(project, base_version=None):
}
if highest_version_obj:
ret_val['url'] = highest_version_obj.get_absolute_url()
ret_val['slug'] = highest_version_obj.slug,
ret_val['slug'] = highest_version_obj.slug
Copy link
Member

Choose a reason for hiding this comment

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

Removing this comma produces tests to fail... I had the same problem and I ended up writing

(highest_version_obj.slug, )

Copy link
Member

Choose a reason for hiding this comment

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

I think the test should be fixed. Its expecting a tuple but it should get the slug in sting

Copy link
Member Author

Choose a reason for hiding this comment

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

@safwanrahman agreed -- want to PR it to master, and I'll merge it in here?

if base_version and base_version.slug != LATEST:
try:
base_version_comparable = parse_version_failsafe(
Expand Down
73 changes: 35 additions & 38 deletions readthedocs/search/indexes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import datetime

from elasticsearch import Elasticsearch, exceptions
from elasticsearch.helpers import bulk_index
from elasticsearch.helpers import bulk

from django.conf import settings

Expand Down Expand Up @@ -48,8 +48,6 @@ def get_settings(self, settings_override=None):
'number_of_replicas': settings.ES_DEFAULT_NUM_REPLICAS,
'number_of_shards': settings.ES_DEFAULT_NUM_SHARDS,
'refresh_interval': '5s',
'store.compress.tv': True,
'store.compress.stored': True,
'analysis': self.get_analysis(),
}
if settings_override:
Expand Down Expand Up @@ -139,7 +137,7 @@ def bulk_index(self, data, index=None, chunk_size=500, parent=None,
docs.append(doc)

# TODO: This doesn't work with the new ES setup.
bulk_index(self.es, docs, chunk_size=chunk_size)
bulk(self.es, docs, chunk_size=chunk_size)

def index_document(self, data, index=None, parent=None, routing=None):
doc = self.extract_document(data)
Expand Down Expand Up @@ -220,25 +218,24 @@ def get_mapping(self):
# Disable _all field to reduce index size.
'_all': {'enabled': False},
'properties': {
'id': {'type': 'long'},
'name': {'type': 'string', 'analyzer': 'default_icu'},
'description': {'type': 'string', 'analyzer': 'default_icu'},

'slug': {'type': 'string', 'index': 'not_analyzed'},
'lang': {'type': 'string', 'index': 'not_analyzed'},
'tags': {'type': 'string', 'index': 'not_analyzed'},
'privacy': {'type': 'string', 'index': 'not_analyzed'},
'id': {'type': 'keyword'},
Copy link
Member

@safwanrahman safwanrahman Dec 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is not id is integer or long?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the new ES doesn't let these vary types, or something. I was getting an error, and it doesn't really need to be an ID, so we will just use it as a string like the others.

Copy link
Member

@safwanrahman safwanrahman Dec 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@ericholscher Seems like they support it
https://www.elastic.co/guide/en/elasticsearch/reference/current/number.html

Without ID, what can be here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does seem strange that we're coercing to string here, but I'm not familiar enough with ES to say why token/string is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not that it's better, it just doesn't work otherwise. I don't fully understand it either.

Copy link
Member Author

Choose a reason for hiding this comment

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

It throws this error: RequestError: TransportError(400, u'illegal_argument_exception', u'mapper [id] cannot be changed from type [long] to [keyword]')

'name': {'type': 'text', 'analyzer': 'default_icu'},
'description': {'type': 'text', 'analyzer': 'default_icu'},

'slug': {'type': 'keyword'},
'lang': {'type': 'keyword'},
'tags': {'type': 'keyword'},
'privacy': {'type': 'keyword'},
'author': {
'type': 'string',
'type': 'text',
'analyzer': 'default_icu',
'fields': {
'raw': {
'type': 'string',
'index': 'not_analyzed',
'type': 'keyword',
},
},
},
'url': {'type': 'string', 'index': 'not_analyzed'},
'url': {'type': 'keyword'},
# Add a weight field to enhance relevancy scoring.
'weight': {'type': 'float'},
}
Expand Down Expand Up @@ -273,19 +270,19 @@ def get_mapping(self):
# Disable _all field to reduce index size.
'_all': {'enabled': False},
# Associate a page with a project.
'_parent': {'type': self._parent},
# '_parent': {'type': self._parent},
'properties': {
'id': {'type': 'string', 'index': 'not_analyzed'},
'sha': {'type': 'string', 'index': 'not_analyzed'},
'project': {'type': 'string', 'index': 'not_analyzed'},
'version': {'type': 'string', 'index': 'not_analyzed'},
'path': {'type': 'string', 'index': 'not_analyzed'},
'taxonomy': {'type': 'string', 'index': 'not_analyzed'},
'commit': {'type': 'string', 'index': 'not_analyzed'},

'title': {'type': 'string', 'analyzer': 'default_icu'},
'headers': {'type': 'string', 'analyzer': 'default_icu'},
'content': {'type': 'string', 'analyzer': 'default_icu'},
'id': {'type': 'keyword'},
'sha': {'type': 'keyword'},
'project': {'type': 'keyword'},
'version': {'type': 'keyword'},
'path': {'type': 'keyword'},
'taxonomy': {'type': 'keyword'},
'commit': {'type': 'keyword'},

'title': {'type': 'text', 'analyzer': 'default_icu'},
'headers': {'type': 'text', 'analyzer': 'default_icu'},
'content': {'type': 'text', 'analyzer': 'default_icu'},
# Add a weight field to enhance relevancy scoring.
'weight': {'type': 'float'},
}
Expand Down Expand Up @@ -321,7 +318,7 @@ def get_mapping(self):
# Disable _all field to reduce index size.
'_all': {'enabled': False},
# Associate a section with a page.
'_parent': {'type': self._parent},
# '_parent': {'type': self._parent},
# Commenting this out until we need it.
# 'suggest': {
# "type": "completion",
Expand All @@ -330,18 +327,18 @@ def get_mapping(self):
# "payloads": True,
# },
'properties': {
'id': {'type': 'string', 'index': 'not_analyzed'},
'project': {'type': 'string', 'index': 'not_analyzed'},
'version': {'type': 'string', 'index': 'not_analyzed'},
'path': {'type': 'string', 'index': 'not_analyzed'},
'page_id': {'type': 'string', 'index': 'not_analyzed'},
'commit': {'type': 'string', 'index': 'not_analyzed'},
'title': {'type': 'string', 'analyzer': 'default_icu'},
'content': {'type': 'string', 'analyzer': 'default_icu'},
'id': {'type': 'keyword'},
'project': {'type': 'keyword'},
'version': {'type': 'keyword'},
'path': {'type': 'keyword'},
'page_id': {'type': 'keyword'},
'commit': {'type': 'keyword'},
'title': {'type': 'text', 'analyzer': 'default_icu'},
'content': {'type': 'text', 'analyzer': 'default_icu'},
'blocks': {
'type': 'object',
'properties': {
'code': {'type': 'string', 'analyzer': 'default_icu'}
'code': {'type': 'text', 'analyzer': 'default_icu'}
}
},
# Add a weight field to enhance relevancy scoring.
Expand Down
1 change: 1 addition & 0 deletions readthedocs/settings/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ def INSTALLED_APPS(self): # noqa
if donate:
apps.append('django_countries')
apps.append('readthedocsext.donate')
apps.append('readthedocsext.search')
Copy link
Member

Choose a reason for hiding this comment

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

Why is this under if donate:?

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed that here, actually: https://github.com/rtfd/readthedocs.org/pull/3356/files#diff-5973fe59a4d52278d55f884d145bd5d7R11 :) Waiting on it to get merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Merged #3356, this can be rebased

return apps

TEMPLATE_LOADERS = (
Expand Down
10 changes: 7 additions & 3 deletions readthedocs/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,13 @@

if 'readthedocsext.donate' in settings.INSTALLED_APPS:
# Include donation URL's
groups.append([
url(r'^sustainability/', include('readthedocsext.donate.urls')),
])
groups.insert(0,
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Seems to be the same, anyway.

[url(r'^sustainability/', include('readthedocsext.donate.urls'))]
)
for num, _url in enumerate(rtd_urls):
Copy link
Member

Choose a reason for hiding this comment

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

Same question again. This is only if the donate app is installed, why? I don't find the relationship between donate app and search app.

if _url and hasattr(_url, 'name') and _url.name == 'search':
rtd_urls[num] = \
url(r'^search/', 'readthedocsext.search.mainsearch.elastic_search', name='search')
if not getattr(settings, 'USE_SUBDOMAIN', False) or settings.DEBUG:
groups.insert(0, docs_urls)
if getattr(settings, 'ALLOW_ADMIN', True):
Expand Down
3 changes: 1 addition & 2 deletions requirements/pip.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ dnspython==1.15.0
httplib2==0.7.7

# Search
elasticsearch==1.5.0
pyelasticsearch==0.7.1
elasticsearch==5.5.1
pyquery==1.2.2

# Utils
Expand Down