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

Deprecate percolate query's document_type parameter. #25199

Conversation

Projects
None yet
4 participants
@martijnvg
Copy link
Member

commented Jun 13, 2017

The document_type parameter is no longer required to be specified,
because by default from 6.0 only a single type is allowed. (index.mapping.single_type defaults to true)

@jpountz
Copy link
Contributor

left a comment

I left some comments about corner cases.

modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java Outdated
*/
@Deprecated
public PercolateQueryBuilder(String field, String documentType, BytesReference document) {
this(field, documentType, document, XContentFactory.xContentType(document));
}

public PercolateQueryBuilder(String field, BytesReference document, XContentType documentXContentType) {

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 14, 2017

Contributor

javadocs?

modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java Outdated

ParsedDocument doc = docMapper.parse(source(context.index().getName(), documentType, "_temp_id", document, documentXContentType));
Collection<String> types = mapperService.types();
assert types.size() == 1;

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 14, 2017

Contributor

it could be 0 if the index was just created without types?

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 14, 2017

Contributor

if types.size() == 1 should we throw an exception if documentType is different from type?

This comment has been minimized.

Copy link
@martijnvg

martijnvg Jun 19, 2017

Author Member

it could be 0 if the index was just created without types?

That can happen, but that also mean that there isn't a percolator field in that case we should fail. I'll move the field validation before this part, so that check can remain the same.

if types.size() == 1 should we throw an exception if documentType is different from type?

This if statement only gets executed if index.mapping.single_type is set to true, so then there should be only a 1 type?

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 20, 2017

Contributor

true, but it might not be the same type as the query type?

@martijnvg martijnvg force-pushed the martijnvg:percolator_deprecated_document_type_parameter branch Jun 19, 2017

@martijnvg

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2017

@jpountz I've updated the pr.

modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java Outdated
* @param documentXContentType The content type of the binary blob containing the document to percolate
*/
public PercolateQueryBuilder(String field, BytesReference document, XContentType documentXContentType) {
this(field, "doc", document, documentXContentType);

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 20, 2017

Contributor

should we pass null instead of doc? It should work regardless of what the single type is?

if (types.size() != 1) {
throw new IllegalStateException("Only a single type should exist, but [" + types.size() + " types exists");
}
String type = types.iterator().next();

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 20, 2017

Contributor

we should fail if documentType is not null and documentType != type, which can happen if the query has a different type than the mapping?

This comment has been minimized.

Copy link
@martijnvg

martijnvg Jun 21, 2017

Author Member

Good point. I added validation for this.

@martijnvg martijnvg force-pushed the martijnvg:percolator_deprecated_document_type_parameter branch Jun 21, 2017

docs/reference/migration/migrate_6_0/search.asciidoc Outdated
@@ -53,6 +53,10 @@

* The `template` query has been removed. This query was deprecated since 5.0

* The `percolate` query's `document_type` has been deprecated. From 6.0 and later
it is no longer required to specify `document_type` parameter on indices with

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 21, 2017

Contributor

let's end the sentence after parameter since we plan on not allowing to set this setting at all in 6.x

percolator: Deprecate `document_type` parameter.
The `document_type` parameter is no longer required to be specified,
because by default from 6.0 only a single type is allowed. (`index.mapping.single_type` defaults to `true`)

@martijnvg martijnvg force-pushed the martijnvg:percolator_deprecated_document_type_parameter branch to a977569 Jun 22, 2017

@martijnvg martijnvg merged commit a977569 into elastic:master Jun 22, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is original commit.
Details
CLA Commit author is a member of Elasticsearch
Details
@danielmitterdorfer

This comment has been minimized.

Copy link
Member

commented Jul 14, 2017

@martijnvg I just stumbled upon this change when checking the deprecation log of our benchmarks. In the docs it still says that document_type is a required parameter. Should we change that and maybe even have a short deprecation note for this parameter?

@martijnvg

This comment has been minimized.

Copy link
Member Author

commented Jul 14, 2017

@danielmitterdorfer Thanks for catching this. It should be formulated differently. It is only required for indices created before 6.0. I'll update the docs.

danielmitterdorfer added a commit to elastic/rally-tracks that referenced this pull request Jul 14, 2017

@danielmitterdorfer

This comment has been minimized.

Copy link
Member

commented Jul 14, 2017

Cool, thank you @martijnvg!

@clintongormley clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.