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 _type from LeafDocLookup #37491

Merged
merged 7 commits into from
Jan 16, 2019
Merged

Deprecate _type from LeafDocLookup #37491

merged 7 commits into from
Jan 16, 2019

Conversation

jdconrad
Copy link
Contributor

This change deprecates _type within LeafDocLookup which is currently used by scripts to fetch the type of document. A recent change (#37281) allows this message to be logged from within scripts.

@jdconrad jdconrad added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >deprecation v7.0.0 labels Jan 15, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Left a few comments

@@ -72,6 +79,10 @@ public void setDocument(int docId) {

@Override
public ScriptDocValues<?> get(Object key) {
// deprecate _type
if ("_type".equals(key)) {
DEPRECATION_LOGGER.deprecated(TYPES_DEPRECATION_MESSAGE);
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be deprecatedAndMaybeLog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

public void testTypesDeprecation() {
ScriptDocValues<?> fetchedDocValues = docLookup.get("_type");
assertEquals(docValues, fetchedDocValues);
assertWarnings("[types removal] Looking up doc types in scripts is deprecated.");
Copy link
Member

Choose a reason for hiding this comment

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

If this warning will be written out, then no need for a package private constant in leaf doc lookup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

Looks good to me from the types deprecation side.

@@ -72,6 +79,10 @@ public void setDocument(int docId) {

@Override
public ScriptDocValues<?> get(Object key) {
// deprecate _type
if ("_type".equals(key)) {
DEPRECATION_LOGGER.deprecated(TYPES_DEPRECATION_MESSAGE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use deprecatedAndMaybeLog?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) already done. @rjernst mentioned the same thing.

@jdconrad
Copy link
Contributor Author

@rjernst @jtibshirani Thanks for the reviews. Will commit as soon as CI passes.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM, one last thing

@@ -72,6 +79,10 @@ public void setDocument(int docId) {

@Override
public ScriptDocValues<?> get(Object key) {
// deprecate _type
if ("_type".equals(key)) {
DEPRECATION_LOGGER.deprecatedAndMaybeLog(TYPES_DEPRECATION_MESSAGE, TYPES_DEPRECATION_MESSAGE);
Copy link
Member

Choose a reason for hiding this comment

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

The first argument to deprecatedAndMaybeLog should be an arbitrary key uniquely identifying this deprecation point. Normally it would be something like type-field-doc-lookup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@jdconrad
Copy link
Contributor Author

@elasticmachine test this please

@jdconrad
Copy link
Contributor Author

@elasticmachine run gradle build tests 1

1 similar comment
@jdconrad
Copy link
Contributor Author

@elasticmachine run gradle build tests 1

@jdconrad jdconrad merged commit 3d8c046 into elastic:master Jan 16, 2019
cshtdd added a commit to cshtdd/elasticsearch that referenced this pull request Jan 16, 2019
* master:
  Deprecate _type from LeafDocLookup (elastic#37491)
  Allow system privilege to execute proxied actions (elastic#37508)
  Update Put Watch to allow unknown fields (elastic#37494)
  AwaitsFix testAddNewReplicas
  SQL: Add protocol tests and remove jdbc_type from drivers response (elastic#37516)
  SQL: [Docs] Add an ES-SQL column for data types (elastic#37529)
  IndexMetaData#mappingOrDefault doesn't need to take a type argument. (elastic#37480)
  Simplify + Cleanup Dead Code in Settings (elastic#37341)
  Reject all requests that have an unconsumed body (elastic#37504)
  [Ml] Prevent config snapshot failure blocking migration (elastic#37493)
  Fix line length for aliases and remove suppression (elastic#37455)
  Add SSL Configuration Library (elastic#37287)
  SQL: Remove slightly used meta commands (elastic#37506)
  Simplify Snapshot Create Request Handling (elastic#37464)
  Remove the use of AbstracLifecycleComponent constructor elastic#37488 (elastic#37488)
  [ML] log minimum diskspace setting if forecast fails due to insufficient d… (elastic#37486)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >deprecation v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants