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

Update DED for Elasticsearch 6 #81

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@HansAdema
Copy link
Contributor

HansAdema commented Jan 18, 2018

By popular demand (#78) it's finally here: support for Elasticsearch 6.

The following changes were necessary:

The string field has been removed completely.

With Elasticsearch 5, the string field type was deprecated (#76) in favor of separate text and keyword fields. The previous auto-mapping worked because string was not removed yet but that needed to be changed here.

The new auto mapping defaults are now to use text and keyword field. When using these fields with Elasticsearch DSL 2.x, a string field will be used instead.

Document mapping types are going to be removed.

Document mapping types and the ability to load multiple DocTypes in a single index has been removed with Elasticsearch 6, and trying to create such indexes will throw errors. Indexes created with ES <=5 still work. The tests had to be updated to accommodate this.

More info on the Elastic blog.

The attachment field has been removed completely.

Due to the deprecation (ES 5.x) and subsequent removal (ES 6.x) of the document mapper plugin, the Attachment field is gone as well. The ingest attachment plugin should be used instead.

I implemented a simple polyfill, but I'm not familiar enough with the plugin to verify whether this will work as expected.

DocTypeOptions has been rewritten.

The structure of DocTypeOptions has been rewritten for EDSL 6, so reading object properties works slightly differently. Of course, the code is still backwards compatible.

@HansAdema

This comment has been minimized.

Copy link
Contributor

HansAdema commented Jan 18, 2018

It looks like the CI build has failing. This is caused by a pip error, not a test failure. If you could rerun the build, it should complete successfully.

@sabricot

This comment has been minimized.

Copy link
Owner

sabricot commented Jan 18, 2018

Thank you @HansAdema for all your PRs, i really need to take some time to review everything.

@HansAdema HansAdema force-pushed the HansAdema:elasticsearch-6 branch from f640298 to dad554e Feb 5, 2018

@zhiyonghe

This comment has been minimized.

Copy link

zhiyonghe commented Mar 2, 2018

hi HansAdema :
my env: python 3.6,elasticsearch (6.1.1), elasticsearch-dsl(6.1.0), and Update DED for Elasticsearch 6 ,but when I run ./manage.py search_index --rebuild ,get the follow error:
elasticsearch_dsl.exceptions.IllegalOperation:Index object cannot have multiple types,default already set,trying to to assign doc.
what's the reason and how to solve the issue. thanks.

@HansAdema

This comment has been minimized.

Copy link
Contributor

HansAdema commented Mar 2, 2018

@zhiyonghe Like the message says: Index object cannot have multiple types. Starting with Elasticsearch 6, DocTypes are deprecated and you cannot have multiple types per index.

Please see the Elastic blog link I posted on the original PR message for more info about this.

@zhiyonghe

This comment has been minimized.

Copy link

zhiyonghe commented Mar 2, 2018

I nothing to change, only follow the example to test.
models.py:
from django.db import models
class Car(models.Model):
name = models.CharField()
color = models.CharField()
description = models.TextField()
type = models.IntegerField(choices=[
(1, "Sedan"),
(2, "Truck"),
(4, "SUV"),
])
documents.py:
from django_elasticsearch_dsl import DocType, Index
from .models import Car

car = Index('cars')
car.settings(
number_of_shards=1,
number_of_replicas=0
)

@car.doc_type
class CarDocument(DocType):
class Meta:
model = Car # The model associated with this DocType

        # The fields of the model you want to be indexed in Elasticsearch
        fields = [
            'name',
            'color',
            'description',
            'type',
        ]

To create and populate the Elasticsearch index and mapping use the search_index command::

$ ./manage.py search_index --rebuild

then throws the error: Index object cannot have multiple types
check the es, there no index cars,the command isn't to create in es auto? is there some else to do ?

@HansAdema

This comment has been minimized.

Copy link
Contributor

HansAdema commented Mar 2, 2018

@zhiyonghe And you're sure you haven't defined Index('cars') anywhere else as well?

If you are, then I'd like to test this for myself. Can you please update the formatting of your code so I can copy and run it without having to reverse the Markdown formatting manually?

@rrmerugu

This comment has been minimized.

Copy link

rrmerugu commented Mar 4, 2018

any plans to merge this soon ?

@zhiyonghe

This comment has been minimized.

Copy link

zhiyonghe commented Mar 5, 2018

@HansAdema ,I am sure that I haven't defined Index('cars') anywhere. is there any test results?

models.py:
from django.db import models
class Car(models.Model):
name = models.CharField()
color = models.CharField()
description = models.TextField()
type = models.IntegerField(choices=[
(1, "Sedan"),
(2, "Truck"),
(4, "SUV"),
])

documents.py:
from django_elasticsearch_dsl import DocType, Index
from .models import Car

car = Index('cars')
car.settings(
number_of_shards=1,
number_of_replicas=0
)

@car.doc_type
class CarDocument(DocType):
class Meta:
model = Car # The model associated with this DocType

    # The fields of the model you want to be indexed in Elasticsearch
    fields = [
        'name',
        'color',
        'description',
        'type',
    ]
@HansAdema

This comment has been minimized.

Copy link
Contributor

HansAdema commented Mar 5, 2018

@zhiyonghe The model and document you're using looks a lot like these fixtures which are used in the integration tests, which are tested against an actual Elasticsearch installation.

In short, I cannot reproduce this from my end.

Regardless, defining indexes and tacking DocType classes to them is done by Elasticsearch DSL, not this library. DED doesn't do anything meaningful with indexes which could influence this behavior (as far as I can tell). If possible could you try to strip the Django specific code from your example and ask your question with the Elasticsearch DSL folks.

Also, for future reference, if you paste a block of source code, please wrap it in triple backticks backticks (```) for formatting.

@hrdymchl

This comment has been minimized.

Copy link

hrdymchl commented Mar 6, 2018

+1 for merging please :)

@pmayer

This comment has been minimized.

Copy link

pmayer commented Mar 23, 2018

Is there any chance of merging this soon?

dsanders11 added a commit to dsanders11/django-elasticsearch-dsl that referenced this pull request Apr 12, 2018

dsanders11 added a commit to dsanders11/django-elasticsearch-dsl that referenced this pull request Apr 12, 2018

@@ -50,6 +51,7 @@ class Meta:
'launched',
'type',
]
doc_type = 'car_document'

This comment has been minimized.

@dsanders11

dsanders11 Apr 12, 2018

Contributor

@HansAdema, is this change necessary? Seems redundant with the use of @car_index.doc_type? I've tried this PR in my own code without adding doc_type to Meta and it seems to work fine.

dsanders11 added a commit to dsanders11/django-elasticsearch-dsl that referenced this pull request Apr 12, 2018

@dsanders11

This comment has been minimized.

Copy link
Contributor

dsanders11 commented Apr 13, 2018

@HansAdema, I split out the changes from this PR that were just formatting changes (into #90) in the hopes that it might speed-up getting this merged by making it easier to review this PR, as about 10% of the line changes were just formatting ones. So that PR could be quick to merge then this one could be rebased and there'll be less to review.

@janbuerling

This comment has been minimized.

Copy link

janbuerling commented Apr 16, 2018

I was wondering what the current situation is regarding the pull request? Is there a chance that it is going to pull soon?

@HansAdema @dsanders11 - Could I get some response, please.

@Ripcurl99983

This comment has been minimized.

Copy link

Ripcurl99983 commented Apr 17, 2018

merge please

@sabricot

This comment has been minimized.

Copy link
Owner

sabricot commented Apr 17, 2018

Upon the insistence i guess i have no choice :)
I will take a look at it before the end of the week.

@janbuerling

This comment has been minimized.

Copy link

janbuerling commented Apr 22, 2018

@sabricot excuse me again, but do you think you have the time to dedicate yourself to this pull request today? I would be very grateful :)

sabricot added a commit that referenced this pull request Apr 22, 2018

@sabricot

This comment has been minimized.

Copy link
Owner

sabricot commented Apr 22, 2018

Its merged and released with the 0.5.0. Thanks again to @HansAdema for the great work 👍

@sabricot sabricot closed this Apr 22, 2018

@safwanrahman

This comment has been minimized.

Copy link
Collaborator

safwanrahman commented May 22, 2018

Possible to update the documentation?

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