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

Upgrade ElasticSearch, fix #448 #449

Merged
merged 6 commits into from May 11, 2016

Conversation

Projects
None yet
3 participants
@davidbgk
Member

davidbgk commented Apr 26, 2016

For now, the website is loading correctly but there are still a few issues:

  • restore previous aggregations (sub-aggregations?)
  • fix extras aggregations?

Once done, tests should pass :-)

facets = {
'organization': ModelTermFacet('organizations', Organization),
aggregations = {
'user_organizations': ModelTermFacet('user_organizations', Organization),

This comment has been minimized.

@noirbizarre

noirbizarre Apr 27, 2016

Member

This one might not be backward compatible with frontend facet declaration any more

This comment has been minimized.

@noirbizarre

noirbizarre Apr 27, 2016

Member

for coherence it should be 'user_organization': ModelTermFacet('user_organizations', Organization), (singular)

This comment has been minimized.

@davidbgk

davidbgk Apr 27, 2016

Member

@noirbizarre why singular? We're retrieving many organizations with that facet, it looks more consistent to me like that.

This comment has been minimized.

@noirbizarre

noirbizarre Apr 27, 2016

Member

The name is used in URL so it will be user_organization=orgid same as dataset org, reuse org, tags...

@@ -83,7 +83,8 @@
{% macro facet_panel(result, name, label, icon=None, url=None) %}
{% set facet = result.get_facet(name) %}
{% set facet = result.get_aggregation(name) %}

This comment has been minimized.

@noirbizarre

noirbizarre Apr 27, 2016

Member

I think it might be none for facets/aggs that have been renamed (organization vs user_organization)

@@ -2,7 +2,7 @@
{% set url = search_url %}
<div class="panel panel-default advanced-search-panel">
{{ s.facet_panel(users,
'organization', _('Organizations'), ficon('fa-building-o'), url=url
'user_organizations', _('Organizations'), ficon('fa-building-o'), url=url

This comment has been minimized.

@noirbizarre

noirbizarre Apr 27, 2016

Member

should be singular

davidbgk and others added some commits Apr 26, 2016

Merge branch 'dev' into 448-upgrade-es
# Conflicts:
#	requirements/install.pip
```shell
$ brew install elasticsearch
$ /usr/local/Cellar/elasticsearch/2.3.1/libexec/bin/plugin install analysis-icu
```

This comment has been minimized.

@noirbizarre

noirbizarre May 11, 2016

Member

A Debian/Ubuntu extra example could be nice.

This comment has been minimized.

@davidbgk

davidbgk May 11, 2016

Member

Please provide it ;)

@@ -80,6 +80,6 @@ def pager(page_fields):
'next_page': NextPageUrl(description='The next page URL if exists'),
'previous_page': PreviousPageUrl(
description='The previous page URL if exists'),
'facets': Raw(description='Search facets results if any'),
'aggregations': Raw(description='Search aggregations results if any'),

This comment has been minimized.

@noirbizarre

noirbizarre May 11, 2016

Member

We missed this one: it should be facets

@@ -53,23 +53,25 @@ def display(topic):
def datasets(topic):
kwargs = multi_to_dict(request.args)
kwargs.update(topic=topic)
query = TopicSearchQuery(Dataset, aggregations=True, **kwargs)

This comment has been minimized.

@noirbizarre

noirbizarre May 11, 2016

Member

And this one ;)

)
@blueprint.route('/<topic:topic>/reuses')
def reuses(topic):
kwargs = multi_to_dict(request.args)
kwargs.update(topic=topic)
query = TopicSearchQuery(Reuse, aggregations=True, **kwargs)

This comment has been minimized.

@noirbizarre
@@ -23,7 +23,7 @@
)
ES_NUM_FAILURES = '-Infinity', 'Infinity', 'NaN'
ES_NUM_FAILURES = '-Infinity', 'Infinity', 'NaN', None

This comment has been minimized.

@noirbizarre
@@ -54,7 +54,7 @@
{% for label, endpoint, type in exports %}
<li role="presentation">
<a role="menuitem" tabindex="-1" href="{{ result.query.to_url(
url_for(endpoint), facets=None, page=None, page_size=None, replace=true)
url_for(endpoint), aggregations=None, page=None, page_size=None, replace=true)

This comment has been minimized.

@noirbizarre

noirbizarre May 11, 2016

Member

Should be facets too

@@ -269,7 +269,7 @@ <h3 class="panel-title">
{% if csv_endpoint %}
<a class="btn btn-xs btn-info pull-right"
href="{{ result.query.to_url(
url_for(csv_endpoint), facets=None, page=None, page_size=None, replace=true)
url_for(csv_endpoint), aggregations=None, page=None, page_size=None, replace=true)

This comment has been minimized.

@noirbizarre
@@ -80,8 +80,8 @@ def test_dataset_api_list_with_facets(self):
response = self.get(url_for('api.datasets', **{'facets': 'tag'}))
self.assert200(response)
self.assertEqual(len(response.json['data']), 2)
self.assertIn('facets', response.json)
self.assertIn('tag', response.json['facets'])
self.assertIn('aggregations', response.json)

This comment has been minimized.

@noirbizarre

noirbizarre May 11, 2016

Member

And this one

@@ -196,7 +196,7 @@ def test_resources_csv_with_filters(self):
response = self.get(
url_for('site.resources_csv', tag='selected', page_size=3,
facets=True))
aggregations=True))

This comment has been minimized.

@noirbizarre
url_for(
'site.datasets_csv', tag='selected', page_size=3, facets=True))
url_for('site.datasets_csv', tag='selected', page_size=3,
aggregations=True))

This comment has been minimized.

@noirbizarre

noirbizarre May 11, 2016

Member

Here too

badge=PUBLIC_SERVICE, page_size=3, facets=True))
response = self.get(
url_for('site.organizations_csv', badge=PUBLIC_SERVICE,
page_size=3, aggregations=True))

This comment has been minimized.

@noirbizarre
url_for(
'site.reuses_csv', tag='selected', page_size=3, facets=True))
url_for('site.reuses_csv', tag='selected', page_size=3,
aggregations=True))

This comment has been minimized.

@noirbizarre

@davidbgk davidbgk referenced this pull request May 11, 2016

Closed

448 upgrade es #456

"to delete it manually (e.g. "
"`curl -XDELETE 'http://localhost:9200/_all/'`) and "
"to use the ` search init` command if you want to "
"update the mapping.").format(name=name))

This comment has been minimized.

@noirbizarre

noirbizarre May 11, 2016

Member

I think you can only suggest to use search init which properly handle index deletion.

"to delete it manually (e.g. "
"`curl -XDELETE 'http://localhost:9200/_all/'`) and "
"to use the ` search init` command if you want to "
"update the mapping.").format(name=name))
es.indices.put_mapping(index=name,

This comment has been minimized.

@noirbizarre

noirbizarre May 11, 2016

Member

This one should be conditional now: if the mapping exists, we don't try to add it anymore (as it's not deleted anymore)

if es.indices.exists_type(index=self.index_name,
doc_type=doc_type):
es.indices.delete_mapping(index=self.index_name,
doc_type=doc_type)
es.indices.put_mapping(index=self.index_name,

This comment has been minimized.

@noirbizarre

noirbizarre May 11, 2016

Member

Same here

Do not delete mapping anymore, refs #448
With ES 2.* you cannot anymore delete a given mapping, you have to delete the whole index and recreate it:

https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-delete-mapping.html
@noirbizarre

This comment has been minimized.

Member

noirbizarre commented May 11, 2016

r+wc

@davidbgk davidbgk merged commit 04b7091 into opendatateam:dev May 11, 2016

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@davidbgk davidbgk removed the in progress label May 11, 2016

@davidbgk davidbgk deleted the davidbgk:448-upgrade-es branch May 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment