Adds deprecation logging to ScriptDocValues#getValues. #34279

j-haj · 2018-10-03T17:28:37Z

First commit addressing issue #22919.

ScriptDocValues#getValues was added for backwards compatibility but no
longer needed. Scripts using the syntax doc.foo.values.bar should be
using doc.foo.bar instead.

rjernst

Unfortunately this won't work as is. Due to scripts running with reduced permissions, the call to the logger needs to be wrapped with an AccessController.doPrivileged call. See the other deprecation examples in ScriptDocValues. We also need a test for this, preferably for each subclass.

j-haj · 2018-10-03T18:18:16Z

Thanks for the fast response -- I will review the examples and update accordingly

elasticmachine · 2018-10-04T09:01:50Z

Pinging @elastic/es-core-infra

nik9000 · 2018-10-04T14:23:12Z

It is sneaky how it won't work too. For the most part it only fails when the log call happens to roll the log file.

j-haj · 2018-10-04T16:19:48Z

Ok I've updated with an initial attempt at a fix. I think using the BiConsumer is a little awkward but it seemed clearer than using a standard Consumer and doing some currying to apply the log key to deprecatedAnMaybeLog.

I will think about how testing for each class in the meantime.

server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java

nik9000 · 2018-10-05T15:38:59Z

The fix looks right to me. I agree on the BiConsumer being better here, at least with the field being call deprecationCallback.

rjernst · 2018-10-14T22:50:09Z

I think what is still missing from here is a test for each doc value type.

j-haj · 2018-10-14T23:49:43Z

@rjernst it is at the top of my todo list! Had some other items I had to take care of last week.

j-haj · 2018-10-22T18:37:47Z

Two quick questions:

Should I rebase this PR off of 6.4? My current branch is from master.
What is the preferred style for final variables? I've seen both deprecationLogger and DEPRECATION_LOGGER, although DEPRECATION_LOGGER seems to be more popular and consistent with common Java style (although I am certainly far from an expert here)

Otherwise I hope to have an updated PR with tests in the next day or two. Again, sorry about the delay!

rjernst · 2018-10-22T18:48:39Z

master is the correct branch. We do all PRs against there, and then backport as appropriate.
Final static variables should be in all uppercase, which all deprecation loggers that I know of are. Where have you seen it in camelCase (we should fix any such cases, separately from this PR)?

jasontedor · 2018-10-22T19:03:13Z

Final static variables should be in all uppercase

If and only if they represent constants (e.g., a primitive type, or an immutable reference type like a String). For example, a private static final AtomicInteger representing a counter of foos should be named fooCounter, not FOO_COUNTER.

It's genuinely an interesting discussion point whether or not a deprecation logger represents a constant. I would say that it does not, since it is mutable internal state that is visible.

Given this, I would say the right name is deprecationLogger.

j-haj · 2018-10-22T19:10:32Z

I count about 58 occurrences of deprecationLogger and about 96 DEPRECATION_LOGGER. This PR uses deprecationLogger - I'll leave it for now unless told to change it. If you all want, I can submit a separate issue to spark discussion.

nik9000 · 2018-10-25T16:58:00Z

If you all want, I can submit a separate issue to spark discussion.

We just had the same discussion on another issue. So an issue sounds good to me!

j-haj · 2018-10-25T17:14:09Z

@nik9000 Done. See #34872.

nik9000 · 2018-10-25T18:20:49Z

@nik9000 Done. See #34872.

Thanks!

j-haj · 2018-10-25T21:19:48Z

I've added an initial attempt at the tests. There are a few things I am unsure on:

I only found tests for the concrete classes Dates, Longs, and GeoPoints. However, there are additional concrete implementations which don't have any tests (e.g., Doubles and Boolean). Do we want to test all concrete types?
There are tests for ScriptDocValueDates in the 6.4 branch but not in master. I created the file and per @rjernst 's comment I figure we can handle the merge when the time comes.
There is a lot of duplicate code in the various concrete type tests. There must be a better way to approach this but it isn't obvious to me at the moment.

rjernst

Thanks for the tests! This looks good, except one nit about the deprecation message.

server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java

First commit addressing issue elastic#22919. `ScriptDocValues#getValues` was added for backwards compatibility but no longer needed. Scripts using the syntax `doc['foo'].values` when `doc['foo']` is a list should be using `doc['foo']` instead.

nik9000

Looks good to me.

nik9000 · 2018-11-02T16:53:39Z

@elasticmachine test this please.

He told me to.

…o issue-22919

j-haj · 2018-11-08T21:31:39Z

I merged master into my branch but ./gradlew check is failing at TcpTransportTests#testCompressRequest. I am looking into the cause so I would hold off on rerunning the build.

nik9000 · 2018-11-08T21:36:19Z

I am looking into the cause so I would hold off on rerunning the build.

I've seen this fail once or twice in our builds. I suspect it is flaky these past few days.

…o issue-22919

j-haj · 2018-11-17T00:05:51Z

I have a few more updates I'll push hopefully by Sunday. I merged master again and that seemed to fix the TcpTransportTests errors -- it looks like all that is left is a few occurrences of .values in some of the Painless integration tests.

nik9000 · 2018-11-19T16:28:48Z

Thanks for sticking with this @j-haj!

j-haj · 2018-11-20T20:48:42Z

@nik9000 My pleasure! I'm seeing a few failures in the ZipClientYamlTestSuitIT integration tests that I am still tracking down.

…o issue-22919

j-haj · 2018-11-26T23:14:57Z

I am still seeing some failures when I run gradlew check but am not convinced they are related to this change. The prior failures I was seeing are gone after merging in master.

nik9000 · 2018-11-27T16:04:48Z

@j-haj, what are you seeing fail locally? Everything seems good in CI. I've pulled this locally and am running it on my side just to check.

j-haj · 2018-11-27T19:09:31Z

@nik9000 I am seeing org.elasticsearch.indices.state.RareClusterStateIT.testDelayedMappingPropagationOnReplica fail locally along with the following slow tests:

org.elasticsearch.cluster.routing.PrimaryAllocationIT
org.elasticsearch.discovery.MasterDisruptionIT
org.elasticsearch.search.aggregations.bucket.MinDocCountIT
org.elasticsearch.discovery.DiscoveryDisruptionIT
org.elasticsearch.discovery.ClusterDisruptionIT

nik9000 · 2018-11-27T19:14:05Z

The slow tests shouldn't be a problem. I'll run the failing test a few times locally. I'm 99.9999999% sure that your change has nothing to do with RareClusterStateIT. Given that CI is passing on the PR I'll merge it this afternoon if I can't get RareClusterStateIT to fail locally.

…

On Tue, Nov 27, 2018 at 2:09 PM Jeff Hajewski ***@***.***> wrote: @nik9000 <https://github.com/nik9000> I am seeing org.elasticsearch.indices.state.RareClusterStateIT.testDelayedMappingPropagationOnReplica fail locally along with the following slow tests: org.elasticsearch.cluster.routing.PrimaryAllocationIT org.elasticsearch.discovery.MasterDisruptionIT org.elasticsearch.search.aggregations.bucket.MinDocCountIT org.elasticsearch.discovery.DiscoveryDisruptionIT org.elasticsearch.discovery.ClusterDisruptionIT — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#34279 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANLoreX0GeWvIo5SadeC7gEbUAtBcS2ks5uzY3ygaJpZM4XGgUK> .

j-haj · 2018-11-27T19:21:35Z

@nik9000 sounds great!

`ScriptDocValues#getValues` was added for backwards compatibility but no longer needed. Scripts using the syntax `doc['foo'].values` when `doc['foo']` is a list should be using `doc['foo']` instead. Closes #22919

* master: DOCS Audit event attributes in new format (elastic#35510) Scripting: Actually add joda time back to whitelist (elastic#35965) [DOCS] fix HLRC ILM doc misreferenced tag Add realm information for Authenticate API (elastic#35648) [ILM] add HLRC docs to remove-policy-from-index (elastic#35759) [Rollup] Update serialization version after backport [Rollup] Add more diagnostic stats to job (elastic#35471) Build: Fix gradle build for Mac OS (elastic#35968) Adds deprecation logging to ScriptDocValues#getValues. (elastic#34279) [Monitoring] Make Exporters Async (elastic#35765) [ILM] reduce time restriction on IndexLifecycleExplainResponse (elastic#35954) Remove use of AbstractComponent in xpack (elastic#35394) Deprecate types in search and multi search templates. (elastic#35669) Remove fromXContent from IndexUpgradeInfoResponse (elastic#35934)

nik9000 · 2018-11-28T13:28:08Z

@j-haj, I've merged and backported to 6.x. Thanks so much for working on this!

j-haj · 2018-11-28T17:33:18Z

@nik9000 Do we need a second PR that removes this functionality? If I recall correctly from the initial discussions, there were two steps: 1) add deprecation logging and 2) remove the methods altogether (or is this what you meant by "backported to 6.x"?).

nik9000 · 2018-11-28T17:36:56Z

@j-haj, yes! We do need another PR to remove the functionality. By backporting to 6.x, I mean that I cherry-picked this change onto the 6.x branch, resolved the conflicts, ran the tests, and pushed it. That is what having the v6.6.0 tag here means. So, yes, we do still need the PR to remove the methods. Would you like to work on PR?

j-haj · 2018-11-28T17:38:38Z

@nik9000 yes I will take on that PR if it's available. If there is no issue already I am happy to create one.

rjernst · 2018-11-28T18:32:10Z

No need to create an issue. Just reference this PR and the original issue in the new PR.

nik9000 · 2018-11-28T21:25:41Z

No need to create an issue. Just reference this PR and the original issue in the new PR.

Yeah! We build our release notes using the PRs rather than the issues.

* Adds deprecation logging to ScriptDocValues#getValues. First commit addressing issue #22919. `ScriptDocValues#getValues` was added for backwards compatibility but no longer needed. Scripts using the syntax `doc['foo'].values` when `doc['foo']` is a list should be using `doc['foo']` instead. * Fixes two build errors in #34279 * Removes unused import in ScriptDocValuesDatesTest * Removes used of `.values` in example in diversified-sampler-aggregation.asciidoc * Removes use of .values from painless test. Part of #34279 * Updates tests to use `doc[foo]` syntax rather than `doc[foo].values`. * Removes use of `getValues()` and replaces use of `doc[foo].values` with `doc[foo]`. * Indentation fix. * Remove unnecessary list construction at previous `getValues()` callsite in ScriptDocValues.GeoPoints. * Update migration doc and add link to `getValue` in ScriptDocValues javadoc. * Fix compile * Fix javadoc issue * Removes ScriptDocValues#getValues usage from painless whitelist.

rjernst previously requested changes Oct 3, 2018

View reviewed changes

colings86 added the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Oct 4, 2018

j-haj force-pushed the issue-22919 branch from b186bb7 to b8f383a Compare October 4, 2018 16:13

nik9000 reviewed Oct 5, 2018

View reviewed changes

server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java Outdated Show resolved Hide resolved

j-haj force-pushed the issue-22919 branch from b8f383a to 570c745 Compare October 5, 2018 16:44

j-haj mentioned this pull request Oct 25, 2018

Naming convention for static final variables with mutable internal state #34872

Closed

j-haj force-pushed the issue-22919 branch 2 times, most recently from 970c3e1 to d643b9e Compare October 25, 2018 21:12

rjernst reviewed Oct 29, 2018

View reviewed changes

server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java Outdated Show resolved Hide resolved

j-haj force-pushed the issue-22919 branch from d643b9e to 51a6c6d Compare November 1, 2018 14:55

j-haj force-pushed the issue-22919 branch from 51a6c6d to c5704a4 Compare November 1, 2018 15:33

nik9000 approved these changes Nov 2, 2018

View reviewed changes

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

ffc1903

…o issue-22919

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

7e50713

…o issue-22919

j-haj added 2 commits November 26, 2018 16:43

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

aa4429d

…o issue-22919

Updates tests to use doc[foo] syntax rather than doc[foo].values.

0103405

nik9000 merged commit 49087f1 into elastic:master Nov 27, 2018

nik9000 added >deprecation v7.0.0 v6.6.0 labels Nov 27, 2018

nik9000 mentioned this pull request Nov 28, 2018

Remove ScriptDocValues#getValues #22919

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Adds deprecation logging to ScriptDocValues#getValues. #34279

Adds deprecation logging to ScriptDocValues#getValues. #34279

Conversation

j-haj commented Oct 3, 2018

rjernst left a comment

Choose a reason for hiding this comment

j-haj commented Oct 3, 2018

elasticmachine commented Oct 4, 2018

nik9000 commented Oct 4, 2018

j-haj commented Oct 4, 2018 • edited

nik9000 commented Oct 5, 2018

rjernst commented Oct 14, 2018

j-haj commented Oct 14, 2018

j-haj commented Oct 22, 2018

rjernst commented Oct 22, 2018

jasontedor commented Oct 22, 2018

j-haj commented Oct 22, 2018

nik9000 commented Oct 25, 2018

j-haj commented Oct 25, 2018

nik9000 commented Oct 25, 2018

j-haj commented Oct 25, 2018

rjernst left a comment

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 commented Nov 2, 2018

j-haj commented Nov 8, 2018

nik9000 commented Nov 8, 2018

j-haj commented Nov 17, 2018

nik9000 commented Nov 19, 2018

j-haj commented Nov 20, 2018

j-haj commented Nov 26, 2018

nik9000 commented Nov 27, 2018

j-haj commented Nov 27, 2018

nik9000 commented Nov 27, 2018 via email

j-haj commented Nov 27, 2018

nik9000 commented Nov 28, 2018

j-haj commented Nov 28, 2018

nik9000 commented Nov 28, 2018

j-haj commented Nov 28, 2018

rjernst commented Nov 28, 2018

nik9000 commented Nov 28, 2018

j-haj commented Oct 4, 2018 •

edited