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

RestSqlIT: Attempt to fix #80089 #107955

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

craigtaverner
Copy link
Contributor

Fixes #80089

Based on the comment regarding a leftover async task:

The leftover task is indices:data/read/sql/async/get.

I've added a waitForPendingTasks for this. The test has been muted for over two years, so I think it's time to unmute it and try this out. If we're wrong, we'll see some failures and can mute it again. But leaving it muted forever seems self-defeating.

@craigtaverner craigtaverner added >test Issues or PRs that are addressing/adding tests :Analytics/SQL SQL querying Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Apr 26, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@craigtaverner
Copy link
Contributor Author

It failed once in part-4 and I'm running it again, but even a single failure does not bode well for merging this.

@bpintea
Copy link
Contributor

bpintea commented Apr 29, 2024

It failed once in part-4 and I'm running it again, but even a single failure does not bode well for merging this.

Wondering if the failure was related, though. The change in the cleanup shouldn't affect the results of the tests? They might still fail, but that might another issue.

Copy link
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

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

LGTM.

@craigtaverner
Copy link
Contributor Author

It failed once in part-4 and I'm running it again, but even a single failure does not bode well for merging this.

Wondering if the failure was related, though. The change in the cleanup shouldn't affect the results of the tests? They might still fail, but that might another issue.

Getting empty results (which is what we saw in the failed test) was something I seem to remember from some of the failures in previous cases, so perhaps this is exactly this same issue. I'll run CI once more, and if it passes again, we could consider merging, watching CI for a while, and muting/reverting if we see this again.

@craigtaverner
Copy link
Contributor Author

Failed again with:

testAsyncTextPaginated FAILED
--
  | org.junit.ComparisonFailure: expected:<[text5          \|5
  | text6          \|6
  | text7          \|7
  | text8          \|8
  | text9          \|9
  | ]> but was:<[]>

So it gets an empty result instead of the expected table of results. The numbers make me think this is the second page, with the first having 0..4, and the second 5..9. So one page works and the second page fails. This has failed a couple of times now in the PR for main, but the PR for 7.17 keeps passing on repeated runs of CI. That is curious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/SQL SQL querying auto-backport-and-merge Automatically create backport pull requests and merge when ready Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.13.5 v8.14.1 v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] RestSqlIT testAsyncTextPaginated failing
3 participants