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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 24 additions & 18 deletions packages/kbn-esql-editor/src/esql_editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ import './overwrite.scss';
// for editor width smaller than this value we want to start hiding some text
const BREAKPOINT_WIDTH = 540;

interface Messages {
errors: MonacoMessage[];
warnings: MonacoMessage[];
}

export const ESQLEditor = memo(function ESQLEditor({
query,
onTextLangQueryChange,
Expand Down Expand Up @@ -125,18 +130,12 @@ export const ESQLEditor = memo(function ESQLEditor({
const [abortController, setAbortController] = useState(new AbortController());

// contains both client side validation and server messages
const [editorMessages, setEditorMessages] = useState<{
errors: MonacoMessage[];
warnings: MonacoMessage[];
}>({
const [editorMessages, setEditorMessages] = useState<Messages>({
errors: serverErrors ? parseErrors(serverErrors, code) : [],
warnings: serverWarning ? parseWarning(serverWarning) : [],
});
// contains only client side validation messages
const [clientParserMessages, setClientParserMessages] = useState<{
errors: MonacoMessage[];
warnings: MonacoMessage[];
}>({
const [clientParserMessages, setClientParserMessages] = useState<Messages>({
errors: [],
warnings: [],
});
Expand Down Expand Up @@ -444,28 +443,31 @@ export const ESQLEditor = memo(function ESQLEditor({
};
}, [esqlCallbacks, code]);

const clientParserStatus = clientParserMessages.errors?.length
? 'error'
: clientParserMessages.warnings.length
? 'warning'
: 'success';
const clientParserStatus = getStatusFromMessages(clientParserMessages);

useEffect(() => {
const validateQuery = async () => {
if (editor1?.current) {
const parserMessages = await parseMessages();
setClientParserMessages({
const nextClientParserMessages = {
errors: parserMessages?.errors ?? [],
warnings: parserMessages?.warnings ?? [],
};
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!

});
setClientParserMessages(nextClientParserMessages);
} else {
addQueriesToCache({
queryString: code,
status: clientParserStatus,
});
}
};
if (isQueryLoading || isLoading) {
validateQuery();
addQueriesToCache({
queryString: code,
status: clientParserStatus,
});
}
}, [clientParserStatus, isLoading, isQueryLoading, parseMessages, code]);

Expand Down Expand Up @@ -855,3 +857,7 @@ export const ESQLEditor = memo(function ESQLEditor({

return editorPanel;
});

function getStatusFromMessages(messages: Messages) {
return messages.errors?.length ? 'error' : messages.warnings.length ? 'warning' : 'success';
}
1 change: 0 additions & 1 deletion test/functional/apps/discover/esql/_esql_view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
defaultIndex: 'logstash-*',
enableESQL: true,
};

describe('discover esql view', function () {
before(async () => {
await kibanaServer.savedObjects.cleanStandardList();
Expand Down