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

[ES|QL] An attempt to resolve flaky statuses of ES|QL history items #200949

Closed

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Nov 20, 2024

Summary

This PR is to resolve a race condition in adding items to ES|QL query history.

From the CI failure: should be an error icon for the first history item but it's a green checkmark.
discoveresql discover esql view query history should add a failed query to the h-68a6114f2a586841f7eee1bfb901cac4bcdc7a2cd577786fd85d6851b6dfbd8b

Checklist

@jughosta jughosta added release_note:fix Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Nov 20, 2024
@jughosta jughosta self-assigned this Nov 20, 2024
@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 20, 2024

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

  • Click to trigger kibana-pull-request for this PR!
  • Click to trigger kibana-deploy-project-from-pr for this PR!

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #19 / #getDataUsageMetricsFiltersFromUrlParams should use given relative startDate and endDate values URL params
  • [job] [logs] Jest Integration Tests #6 / ES capabilities for serverless ES returns the correct capabilities

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
esql 195.9KB 196.0KB +82.0B

cc @jughosta

@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7442

[✅] test/functional/apps/discover/esql/config.ts: 100/100 tests passed.

see run history

const nextClientParserStatus = getStatusFromMessages(nextClientParserMessages);
addQueriesToCache({
queryString: code,
status: nextClientParserStatus,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stratoula It seems that there is a race condition in setting a status for the new history item.

Wdyt about this approach?

I don't quite understand the dependency of this useEffect on clientParserStatus and what other use cases which might affect it. Also when editor1?.current could be unavailable?

It might be better if code owners take a close look at how this race condition can be avoided .

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I know about this problem Julia and I have it on my list. I agree it is better for us to have a look but unfortunately there is no capacity atm. Here is an issue to track it #201079 but I am not sure when we wil find time to tackle this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stratoula Alright, thanks!

@jughosta jughosta closed this Nov 21, 2024
@jughosta jughosta deleted the 196866-fix-flaky-history-items branch November 21, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:fix Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL.
Projects
None yet
4 participants