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.

Sign up for GitHub

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

Jump to bottom

[CI] MoreExpressionIT testSpecialValueVariable failing #99156

Closed
edsavage opened this issue Sep 4, 2023 · 10 comments · Fixed by #99667
Closed

[CI] MoreExpressionIT testSpecialValueVariable failing #99156

edsavage opened this issue Sep 4, 2023 · 10 comments · Fixed by #99667
Assignees
@thecoop
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache Team:Core/Infra Meta label for core/infra team >test-failure Triaged test failures from CI

Comments

@edsavage
Copy link
Contributor

edsavage commented Sep 4, 2023

Build scan:
https://gradle-enterprise.elastic.co/s/tvr734jx3dias/tests/:modules:lang-expression:internalClusterTest/org.elasticsearch.script.expression.MoreExpressionIT/testSpecialValueVariable

Reproduction line:

./gradlew ':modules:lang-expression:internalClusterTest' --tests "org.elasticsearch.script.expression.MoreExpressionIT.testSpecialValueVariable" -Dtests.seed=C179C96877ADAAFF -Dtests.locale=nb -Dtests.timezone=Europe/Saratov -Druntime.java=20

Applicable branches:
main

Reproduces locally?:
No

Failure history:
https://gradle-enterprise.elastic.co/scans/tests?tests.container=org.elasticsearch.script.expression.MoreExpressionIT&tests.test=testSpecialValueVariable

Failure excerpt:

java.lang.AssertionError: expected:<0.7> but was:<0.299999976158142>

  at __randomizedtesting.SeedInfo.seed([C179C96877ADAAFF:893068755394F34B]:0)
  at org.junit.Assert.fail(Assert.java:88)
  at org.junit.Assert.failNotEquals(Assert.java:834)
  at org.junit.Assert.assertEquals(Assert.java:553)
  at org.junit.Assert.assertEquals(Assert.java:683)
  at org.elasticsearch.script.expression.MoreExpressionIT.testSpecialValueVariable(MoreExpressionIT.java:496)
  at jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
  at java.lang.reflect.Method.invoke(Method.java:578)
  at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1758)
  at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:946)
  at com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:982)
  at com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:996)
  at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
  at org.apache.lucene.tests.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:48)
  at org.apache.lucene.tests.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
  at org.apache.lucene.tests.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:45)
  at org.apache.lucene.tests.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:60)
  at org.apache.lucene.tests.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:44)
  at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
  at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:390)
  at com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:843)
  at com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:490)
  at com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:955)
  at com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:840)
  at com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:891)
  at com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:902)
  at org.apache.lucene.tests.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
  at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
  at org.apache.lucene.tests.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:38)
  at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
  at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
  at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
  at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
  at org.apache.lucene.tests.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:53)
  at org.apache.lucene.tests.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:43)
  at org.apache.lucene.tests.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:44)
  at org.apache.lucene.tests.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:60)
  at org.apache.lucene.tests.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:47)
  at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
  at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:390)
  at com.carrotsearch.randomizedtesting.ThreadLeakControl.lambda$forkTimeoutingTask$0(ThreadLeakControl.java:850)
  at java.lang.Thread.run(Thread.java:1623)
@edsavage edsavage added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >test-failure Triaged test failures from CI labels Sep 4, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Sep 4, 2023
@thecoop
Copy link
Member

thecoop commented Sep 5, 2023 •
edited
Loading

This does fail reasonably reliably with test seed C179C96877ADAAFF

@thecoop
Copy link
Member

thecoop commented Sep 5, 2023 •
edited
Loading

So, this is an odd one...

Looks like the test only works coincidentally. What is happening is that the source values are getting mixed up, leading to either the smallest or the largest values getting dropped. It's only looking for a min & max, so most of the time it finds what its looking for. But the stats execution is dodgy.

In ExpressionAggregationScript each script value is set from each doc, but it's being set on the same instance. This value is then re-used when running the script. The exact order each doc is executed affects the outcome.

Execution that succeeds:

Set 10.0 on ReplaceableConstDoubleValueSource2096606171
Set 13.0 on ReplaceableConstDoubleValueSource2096606171
Read 39.0
Read 39.0
Set 5.0 on ReplaceableConstDoubleValueSource2096606171
Read 15.0

Execution that fails:

Set 13.0 on ReplaceableConstDoubleValueSource984741442
Set 10.0 on ReplaceableConstDoubleValueSource984741442
Read 30.0
Read 30.0
Set 5.0 on ReplaceableConstDoubleValueSource984741442
Read 15.0

@thecoop
Copy link
Member

thecoop commented Sep 5, 2023

Looks like the threading has changed - it passes reliably on 8.10, with the following traces:

Set 5.0 on ReplaceableConstDoubleValueSource2005362459
Read 15.0
Set 10.0 on ReplaceableConstDoubleValueSource2005362459
Read 30.0
Set 13.0 on ReplaceableConstDoubleValueSource2005362459
Read 39.0

@thecoop
Copy link
Member

thecoop commented Sep 5, 2023

A bisect points towards #98567 causing this behaviour change - the concurrency has changed

@thecoop
Copy link
Member

thecoop commented Sep 5, 2023

@javanna @JVerwolf Is this just another test that needs marking as non-concurrent? Does this point to a deeper problem somewhere?

@thecoop thecoop self-assigned this Sep 5, 2023
@JVerwolf
Copy link
Contributor

JVerwolf commented Sep 5, 2023

Thanks for finding and investigating this @thecoop!

Some background:
#98567 enabled concurrency by default in just the test code. The purpose of #98567 was to flush out concurrency issues (like this), since they can be difficult to find due to race conditions, etc. I had hoped that I had already found them all, but alas.

Does this point to a deeper problem somewhere?

The problem could be in the way the test is constructed, but it is most likely a real issue that will require investigation.
In the interim, we can disable concurrency for this test so that we don't have flakey builds. I'll open a PR to do that and create an issue to fix the underlying problem.

@JVerwolf JVerwolf mentioned this issue Sep 5, 2023
Open
@JVerwolf
Copy link
Contributor

JVerwolf commented Sep 5, 2023

I created #99203 to disable concurrency for this test so that we don't have flakey test runs.

We may want to keep this issue open for fixing the underlying cause, however.

@javanna javanna mentioned this issue Sep 6, 2023
Closed
@davidkyle
Copy link
Member

Another failure https://gradle-enterprise.elastic.co/s/hzzgrag4rxslk/tests/task/:modules:lang-expression:internalClusterTest/details/org.elasticsearch.script.expression.MoreExpressionIT/testSpecialValueVariable?top-execution=1

@davidkyle davidkyle mentioned this issue Sep 11, 2023
Merged
davidkyle added a commit that referenced this issue Sep 11, 2023
@davidkyle
Mute MoreExpressionIT::testStringSpecialValueVariable (#99411)
af4b2d9 
For #99156
@luigidellaquila
Copy link
Contributor

One more failure today.
I was about to mute this test, but I see there is #99203 in progress.
cc. @JVerwolf

javanna added a commit to javanna/elasticsearch that referenced this issue Sep 19, 2023
@javanna
Adjust ExpressionAggregationScript to support inter-segment concurrency
ddf11cc 
Handling of _value in a script agg does not support search concurrency
when using the expression script engine. The reason is that the value gets set
globally assuming sequential execution. This commit addresses that by setting
the value to the values source associated with the correct leaf reader context,
while it was previosly being set on a shared data structure.

Closes elastic#99156
@javanna javanna mentioned this issue Sep 19, 2023
Merged
@pxsalehi pxsalehi mentioned this issue Sep 20, 2023
Merged
javanna added a commit that referenced this issue Sep 21, 2023
@javanna
Adjust ExpressionAggregationScript to support inter-segment concurren…
75b6f96 
…cy (#99667)

Handling of _value in a script agg does not support search concurrency
when using the expression script engine. The reason is that the value gets set
globally assuming sequential execution. This commit addresses that by setting
the value to the values source associated with the correct leaf reader context,
while it was previosly being set on a shared data structure.

Closes #99156
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@thecoop thecoop

Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache Team:Core/Infra Meta label for core/infra team >test-failure Triaged test failures from CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Adjust ExpressionAggregationScript to support inter-segment concurrency javanna/elasticsearch
6 participants
@luigidellaquila @davidkyle @thecoop @JVerwolf @edsavage @elasticsearchmachine