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

The include_type_name parameter for 6.7 #169

Closed
djzort opened this issue May 1, 2019 · 21 comments
Closed

The include_type_name parameter for 6.7 #169

djzort opened this issue May 1, 2019 · 21 comments

Comments

@djzort
Copy link

djzort commented May 1, 2019

Pertaining to the include_type_name parameter as described at https://www.elastic.co/guide/en/elasticsearch/reference/6.7/removal-of-types.html

Despite reviewing the pod (and even grepping the tarball) I am unable to determine how i can supply this parameter and satiate elasticsearch's warnings.

Could someone please let me know how i can provide it to Search::Elasticsearch?

@clintongormley
Copy link
Contributor

I'm working on an updated release which will support this. In the meantime, you can use a low level method to pass in an unvalidated request:

$es->transport->perform_request($request);

@sewi-cpan
Copy link
Contributor

Any expected release date for the fixed client (or a client without these checks as the server does them anyway and more correct)?

@eanicom
Copy link

eanicom commented Jul 23, 2019

I'm also curious when this fix is expected in the perl module.

@djzort
Copy link
Author

djzort commented Jul 23, 2019

@clintongormley would it be possible to release a tiny minor release to include just this function?

@brostad
Copy link

brostad commented Aug 23, 2019

I just came across this issue too, in the process of testing our indexer against a 6.8 cluster. Without the support for include_type_name I will have to look closer at our code base before considering the upgrade from 6.5.

@sewi-cpan
Copy link
Contributor

The issue is currently blocking Elasticsearch upgrades.😔 Any solution would be great.

@djzort
Copy link
Author

djzort commented Nov 25, 2019

Please please make a tiny minor release

@djzort
Copy link
Author

djzort commented Jan 10, 2020

#178 looks promising

@ezimuel
Copy link
Contributor

ezimuel commented Mar 11, 2020

@sewi-cpan, @djzort, @brostad, @eanicom I just released a dev version 6.80_1 with all the updated APIs for Elasticsearch 6.8.7. Can you check it and let me know? I would like to have some feedback before releasing a stable. Thanks!

@djzort
Copy link
Author

djzort commented Mar 11, 2020

@ezimuel so far so good.

@brostad
Copy link

brostad commented Mar 12, 2020

Just downloaded the new version, thank you @ezimuel!

@truj
Copy link

truj commented Mar 17, 2020

Hi,

I have just tested 6.80_1.
All our unit tests still work but the deprication warning is still there.

From our log:

curl -XHEAD 'http://localhost:9200/our_test_index?pretty=true'

[DEPRECATION] [types removal] The parameter include_type_name should be explicitly specified in get indices requests to prepare for 7.0. In 7.0 include_type_name will default to 'false', which means responses will omit the type name in mapping definitions. - In request: {body => undef,ignore => [],method => "HEAD",path => "/our_test_index",qs => {},serialize => "std"}
# Response: 200, Took: 5 ms
# 1

If I include the parameter include_type_name, I get an Unknown Parameter exception.

perl -MJSON -MSearch::Elasticsearch -le 'print $Search::Elasticsearch::VERSION; my $es = Search::Elasticsearch->new({userinfo => "USERNAME:PASSWORD",nodes => ["OUR_NODE:9201"], trace_to => "Stderr", log_to => "Stderr", deprecate_to => "Stderr"}); print $es->indices->exists(index => "our_test_index", include_type_name => JSON::true());'
6.80_1
Current cxns: ["http://OUR_NODE:9201"]

Forcing ping before next use on all live cxns

Ping [http://OUR_NODE:9201] before next request

[Param] ** Unknown param (include_type_name) in (exists) request. , called from sub Search::Elasticsearch::Role::Client::Direct::__ANON__ at -e line 1.

[Param] ** Unknown param (include_type_name) in (exists) request. , called from sub Search::Elasticsearch::Role::Client::Direct::__ANON__ at -e line 1.

@ezimuel
Copy link
Contributor

ezimuel commented Mar 25, 2020

The include_type_name parameter is available only for the following endpoints:

  • indices->create()
  • indices->get_field_mapping()
  • indices->get()
  • indices->get_mapping()
  • indices->get_template()
  • indices->put_mapping()
  • indices->put_template()
  • indices->rollover()

@truj in your example, you can use $es->indices->get(index => "our_test_index", include_type_name => JSON::true()); to remove the DEPRECATION message.

More information here.

@brostad
Copy link

brostad commented Mar 25, 2020

Thank you for making the Search::Elasticsearch 6.80_1 release!

It has taken me a while (Corona virus etc) to get my act together and update my test suite with the new include_name_type parameter. And all my tests ran smoothly with the exception of one test where I still got deprecation warnings, in it I'm making a call to $e->indices->exists() to check the existence of a named index.

Why would $e->indices->exists() need to know about types? And why isn't the include_type_name parameter available for the exists() method?

For now I have made a workaround by replacing the call to exists() with one to get() and then catch the index_not_found_exception if the index doesn't exist. It's not that pretty but hopefully just temporary fix until we migrate to ES7 later this year.

Anyway, I'm really happy with the 6.80_1 release you've created as all my tests now pass :)

bernt@stagedev:~/bernt/esdev$ prove lib/Retriever/ElasticSearch/t/
lib/Retriever/ElasticSearch/t/IndexName.t .. ok
lib/Retriever/ElasticSearch/t/Indexer.t .... ok
All tests successful.
Files=2, Tests=13, 10 wallclock secs ( 0.09 usr 0.01 sys + 3.07 cusr 0.38 csys = 3.55 CPU)
Result: PASS

Thank you very much @ezimuel and keep up the great work!

@truj
Copy link

truj commented Mar 26, 2020

Thanks for your suggestion, @ezimuel.
I've tried it out, and it works.
But I don't want to use this in our productive environment.

We use the exists() method very often. It results in a simple HEAD without much overhead as only the HTTP header is returned.
If we use $es->indices->get(), this is a GET request returning about 21 kB of data every time.

So I have tried out how the HEAD request works with or without include_type_name.
Without the parameter I get the deprecation warning, as expected:

curl -XHEAD -i -I "http://user:secret@our-es-host:9201/our_test_index"
HTTP/1.1 200 OK
Server: nginx/1.10.3
Date: Thu, 26 Mar 2020 15:25:27 GMT
Content-Type: application/json; charset=UTF-8
Content-Length: 9150
Connection: keep-alive
X-Opaque-Id: 10.35.150.2:537609
Warning: 299 Elasticsearch-6.7.2-56c6e48 "[types removal] The parameter include_type_name should be explicitly specified in get indices requests to prepare for 7.0. In 7.0 include_type_name will default to 'false', which means responses will omit the type name in mapping definitions."

But with the parameter I don't get the warning:

curl -XHEAD -i -I "http://user:secret@our-es-host:9201/our_test_index?include_type_name=true"
HTTP/1.1 200 OK
Server: nginx/1.10.3
Date: Thu, 26 Mar 2020 15:26:06 GMT
Content-Type: application/json; charset=UTF-8
Content-Length: 9150
Connection: keep-alive
X-Opaque-Id: 10.35.150.25:37627

That means that the server side supports this.
So it should be possible to fix it in the perl module as well.

@ezimuel
Copy link
Contributor

ezimuel commented Mar 26, 2020

@truj thanks for reporting this. I investigated more and discovered that the issue seems to be related only for Elasticsearch 6.7. The API specification for indices.exists does not report it, see here. I opened issue elastic/elasticsearch#54292.
Interesting, using Elasticsearch 6.8 the problem disappears thanks to this fix.

I'll update you asap and I think I'll add include_type_name in the next patch release.

@brostad
Copy link

brostad commented Mar 26, 2020

That would awesome! Thank you, @ezimuel !

@ezimuel
Copy link
Contributor

ezimuel commented Jun 24, 2020

@brostad, @truj, @djzort I fixed this issue with #191. When merged, I'll release it with elasticsearch-perl 6.81.

@brostad
Copy link

brostad commented Jun 25, 2020

Wonderful! Thank you very much, I will download 6.81 when it's available :)

ezimuel added a commit that referenced this issue Jun 26, 2020
Fix for #169, adding include_type_name in indices.exists
@ezimuel
Copy link
Contributor

ezimuel commented Jun 26, 2020

Just released 6.81. Updated also CPAN here: https://metacpan.org/release/EZIMUEL/Search-Elasticsearch-6.81

@ezimuel ezimuel closed this as completed Jun 26, 2020
@brostad
Copy link

brostad commented Jun 26, 2020

Thank you, @ezimuel! Have downloaded 6.81 and will give it a test over the weekend.

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

No branches or pull requests

7 participants