Delete deprecated getValues from ScriptDocValues

[j-haj]

Second PR for #22919. This PR removes the following:

• getValues method from ScriptDocValues abstract class
• References to getValues from the concrete subclasses
• References to doc[foo].values (replaced with doc[foo])
• References to getValues throughout the code base (in reference to a subclass of ScriptDocValues)
• Updates appropriate tests

[elasticmachine]

Pinging @elastic/es-core-infra

[nik9000]

Could you add a note to docs/reference/migration/migrate_7_0/scripting.asciidoc to call out this removal?

[nik9000]

This change we probably should backport!

[nik9000]

Thanks for doing this @j-haj! That change to getLats and getLon looks like I should backport it. The rest looks great. Could you add a note to that migration file that I mentioned in my review notes? That is the last part that we need to get this full removed!

[j-haj]

@nik9000 Great! I will make the requested updates today

[j-haj]

@nik9000 in the migration file, what versions should I use for deprecated and removed? Deprecated in 6.6, removed 7.0? I don't have a good sense for Elastic release cycles.

[nik9000]

@nik9000 in the migration file, what versions should I use for deprecated and removed? Deprecated in 6.6, removed 7.0? I don't have a good sense for Elastic release cycles.

Those are exactly the right versions. We'll branch master into 7.x and 7.0 at some point and start releasing from them. We're intentionally pretty quiet about when release because we don't like to disappoint folks. But you can be quite sure that something in the 6.6 branch is bound to be released in 6.6.0 and that master will become 7.0.0.

[nik9000]

@j-haj I pushed a small change to fix the compilation after your javadoc change and merged master.

[nik9000]

@elasticmachine, test this please

[j-haj]

@nik9000 that's embarrassing - I should have caught that... looking at the logs it looks like I missed some getValues calls as well. When I ran the tests locally they failed on the same YAML tests the previous PR failed on for me but I didn't see anything else. Anyways, I will push an update later today.

[nik9000]

Thanks @j-haj! I'm sorry the build is so bumpy now. Our CI is having trouble being green and we have a few folks fighting with it today and yesterday.

[j-haj]

I merged master locally and am now seeing failures from org.elasticsearch.gradle.BuildExamplePluginsIT. @nik9000 do you have any suggestions of what I should look into to see if these are due to my changes or due to the recently flaky tests?

[nik9000]

@j-haj, push the merge and I can try it and look at the error.

[nik9000]

Let's see what @elasticmachine thinks: test this please

[j-haj]

Looks like an issue with the Jenkins build script

[nik9000]

@elasticmachine, retest this please. You had problems that last time you tried.

[nik9000]

@j-haj, I'm not sure what is up with the build here. Locally I get the same failures. I tracked the failure down by doing:

cat plugins/examples/painless-whitelist/build/cluster/integTestCluster\ node0/elasticsearch-7.0.0-SNAPSHOT/logs/example-plugins_painless-whitelist_integTestCluster.log

Which had an error that made me think there'd be an easier way to reproduce. So I did:

./gradlew -p modules/lang-painless/ check

And, like magic, the first test failed with:

   > Throwable #1: java.lang.IllegalArgumentException: error loading whitelist(s) [org.elasticsearch.txt]:[70]
   >    at __randomizedtesting.SeedInfo.seed([1FFEF6AE88BDC056:6DC8750D34DF4B80]:0)
   >    at org.elasticsearch.painless.lookup.PainlessLookupBuilder.buildFromWhitelists(PainlessLookupBuilder.java:174)
   >    at org.elasticsearch.painless.DefBootstrapTests.<init>(DefBootstrapTests.java:36)
   >    at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
   >    at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
   >    at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
   >    at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490)
   >    at java.base/java.lang.Thread.run(Thread.java:834)
   > Caused by: java.lang.IllegalArgumentException: reflection object not found for method [[org.elasticsearch.index.fielddata.ScriptDocValues.Strings], [getValues], []]
   >    at org.elasticsearch.painless.lookup.PainlessLookupBuilder.addPainlessMethod(PainlessLookupBuilder.java:509)
   >    at org.elasticsearch.painless.lookup.PainlessLookupBuilder.addPainlessMethod(PainlessLookupBuilder.java:450)
   >    at org.elasticsearch.painless.lookup.PainlessLookupBuilder.buildFromWhitelists(PainlessLookupBuilder.java:138)
   >    ... 24 more
   > Caused by: java.lang.NoSuchMethodException: org.elasticsearch.index.fielddata.ScriptDocValues$Strings.getValues()
   >    at java.base/java.lang.Class.getMethod(Class.java:2109)
   >    at org.elasticsearch.painless.lookup.PainlessLookupBuilder.addPainlessMethod(PainlessLookupBuilder.java:506)

Which is totally related to your change and fixable by removing that method from the painless whitelist.

Now, all of the other painless tests failed with a can't load a class error because class loading bailed on the previous error. Which isn't great, but at least I found the error.

[j-haj]

Thanks! I will fix this now and see what happens

[j-haj]

I made the change and did a clean build and am now rerunning the tests (for about the past 2 hours). So far so good...

[j-haj]

@nik9000 All tests are passing locally on the latest push

[nik9000]

@elasticmachine, test this please!

[nik9000]

@elasticmachine, test this again now that I've merged master into it.

[nik9000]

@elasticmachine, test this please

[nik9000]

And merged after everything passed! Thanks @j-haj!