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 types in count and msearch. #35421

Merged
merged 6 commits into from Nov 16, 2018

Conversation

jtibshirani
Copy link
Contributor

@jtibshirani jtibshirani commented Nov 9, 2018

This PR deprecates types in the count and multi-search APIs, and adds tests for types deprecation in the search API.

For each API, the following updates were made:

  • Add deprecation warnings to Rest*Action, plus tests in Rest*ActionTests.
  • For each REST yml test, create one version without types, and another legacy version that retains types (called *_with_types.yml).
  • Deprecate relevant methods on the Java HLRC requests/ responses.
  • Update documentation (for both the REST API and Java HLRC).

For the Java HLRC, I decided not to duplicate each request/ response class as we had discussed in #34041. Instead, this PR continues to use the transport classes, and follows the traditional path of deprecating methods. My reasoning for this choice:

  • Although the Java HLRC is in 'beta', switching to new request/ response classes would be breaking for many early users of the client. I think it would be great to avoid additional breaks if possible, as users already have a lot to think about in 7.0 with types removal, the transport client migration, hit count tracking, etc.
  • Duplicating each of the requests/ responses would add a substantial amount of work to the types deprecation effort, which is already quite an extensive project.

@jtibshirani jtibshirani added the :Search/Mapping Index mappings, including merging and defining field types label Nov 9, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@jtibshirani jtibshirani changed the title Deprecate types in search-related APIs. Deprecate types in count, explain, and msearch. Nov 9, 2018
@jtibshirani jtibshirani force-pushed the deprecate-types-for-search branch 2 times, most recently from c63f05c to a1a3194 Compare November 9, 2018 19:46
@jtibshirani
Copy link
Contributor Author

@javanna @hub-cap would you be able to take a look at this from the perspective of the Java HLRC? I know we discussed creating a new set of classes for each request/ response in #34041, but it looks like we can keep most of the APIs as they are and follow the traditional path of deprecating methods. I think it would be great to avoid additional breaks if possible, as users already have a lot to think about in 7.0 with the types removal, the Java HLRC migration, hit count tracking, etc.

@jtibshirani jtibshirani added the WIP label Nov 9, 2018
@jtibshirani jtibshirani force-pushed the deprecate-types-for-search branch 5 times, most recently from dc226ae to dfe42a3 Compare November 9, 2018 23:42
@jtibshirani jtibshirani changed the title Deprecate types in count, explain, and msearch. Deprecate types in count and msearch. Nov 9, 2018
@@ -151,9 +155,6 @@ public static void parseSearchRequest(SearchRequest searchRequest, RestRequest r
}

String types = request.param("type");
if (types != null) {
deprecationLogger.deprecated("The {index}/{type}/_search endpoint is deprecated, use {index}/_search instead");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need keeping? It warns when invoking URLs of this form:

 GET my_index/_search?type=my_type

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not for this PR but while we're itemising the ways clients can filter by type this currently also raises no warnings:

GET my_index/_search?q=_type:my_type

or other forms of query:

GET my_index/_search
{
  "query":{
	"match":{
	  "_type":"my_type"
	}
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @markharwood! I hadn't considered query parameters at all in this PR, will fix that. I added a note about queries that use _type as a field name to #35190.

@jtibshirani jtibshirani force-pushed the deprecate-types-for-search branch 3 times, most recently from 7fa516e to 8b256ff Compare November 14, 2018 22:34
@jtibshirani jtibshirani removed the WIP label Nov 14, 2018
@jtibshirani
Copy link
Contributor Author

@markharwood I think this is ready for another look. Also tagging @jpountz in case he would like to review.

@markharwood
Copy link
Contributor

Is the plan to add _msearch/template checking into this PR too?

@jtibshirani
Copy link
Contributor Author

No, I was planning to address the search template endpoints separately.

Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

I think its perfectly reasonable to do what you did here. I totally agree with your reasoning. :shipit: !!! (well, ship the HLRC piece, ill let someone else review the server side ;) )

@hub-cap hub-cap mentioned this pull request Nov 15, 2018
@markharwood
Copy link
Contributor

LGTM

@jtibshirani jtibshirani merged commit c6a0904 into elastic:master Nov 16, 2018
@jtibshirani jtibshirani deleted the deprecate-types-for-search branch November 16, 2018 21:04
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Nov 17, 2018
* master: (59 commits)
  SQL: Move internals from Joda to java.time (elastic#35649)
  Add HLRC docs for Get Lifecycle Policy (elastic#35612)
  Align RolloverStep's name with other step names (elastic#35655)
  Watcher: Use joda method to get local TZ (elastic#35608)
  Fix line length for org.elasticsearch.action.* files (elastic#35607)
  Remove use of AbstractComponent in server (elastic#35444)
  Deprecate types in count and msearch. (elastic#35421)
  Refactor an ambigious TermVectorsRequest constructor. (elastic#35614)
  [Scripting] Use Number as a return value for BucketAggregationScript (elastic#35653)
  Removes AbstractComponent from several classes (elastic#35566)
  [DOCS] Add beta warning to ILM pages. (elastic#35571)
  Deprecate types in validate query requests. (elastic#35575)
  Unmute BuildExamplePluginsIT
  Revert "AwaitsFix the RecoveryIT suite - see elastic#35597"
  Revert "[RCI] Check blocks while having index shard permit in TransportReplicationAction (elastic#35332)"
  Remove remaining line length violations for o.e.action.admin.cluster (elastic#35156)
  ML: Adjusing BWC version post backport to 6.6 (elastic#35605)
  [TEST] Replace fields in response with actual values
  Remove usages of CharSequence in Sets (elastic#35501)
  AwaitsFix the RecoveryIT suite - see elastic#35597
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>deprecation :Search/Mapping Index mappings, including merging and defining field types v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants