-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[App Search] Refactor empty engine polling to EngineLogic #103041
Conversation
+ update selector tests with mock engines to just an `engine` var (I saw Jason do this down below for `searchKey` and liked it, so I copied it) + update Documents & Search UI pages to new selectors
- Per Casey, polling is primarily only needed to dynamically update from empty state to non-empty state, which we're going to handle at the engine level in the next commit
- Now that both Engines Overview, Documents, and Search UI views have empty states that are hooked up to EngineLogic, this poll will update all of the above pages automatically when a new document comes in!
- to match other create page header buttons
pollEmptyEngine(); | ||
|
||
return () => { | ||
stopPolling(); | ||
clearEngine(); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flashErrorToast(POLLING_ERROR_TITLE, { | ||
text: POLLING_ERROR_TEXT, | ||
toastLifeTimeMs: POLLING_DURATION * 0.75, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a screencap of this toast behavior:
Quick UX note: This toast won't appear when Enterprise Search is down (because then the app 502s and the generic Error Connecting view gets shown). Instead, this toast occurs when Kibana is down and the user has a stale/cached frontend (hence the screencap briefly showing my terminal, where I stop Kibana). It's fairly unlikely and 404/401 redirects are going to be the most common case, but I thought I should add in something as a generic 5xx catch.
@elasticmachine merge upstream |
|
||
useEffect(() => { | ||
setEngineName(engineNameFromUrl); | ||
initializeEngine(); | ||
return clearEngine; | ||
pollEmptyEngine(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider making pollEmptyEngine
a side effect of calling initializeEngine
?
Currently, the UI needs to call initializeEngine
, call pollEmptyEngine
, and also call stopPolling
. It'd be great if the UI could just call initializeEngine
and the polling automatically started and stopped when appropriate.
initializeEngine:
- Fetch Engine data
- If the Engine is empty, and not already polling, start polling
- If the engine is not empty, stop polling
You could use an unmount
hook in Kea to ensure that polling is cancelled whenever the logic unmounts: https://kea.js.org/docs/guide/additional/#events
Or better yet
Maybe we don't even need to consider the polling state...
initializeEngine:
- Fetch Engine data
- If the Engine is empty, wait x second and call initializeEngine again
Just thinking out loud.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always personally waffle between making things more explicit and easier for devs to follow in code, or making things "magical" (side effects) and less code but potentially harder to follow when reading/debugging.
You could use an unmount hook in Kea to ensure that polling is cancelled whenever the logic unmounts: https://kea.js.org/docs/guide/additional/#events
FYI, we can't do this. I tried this initially, and what's happening is EngineLogic is not fully unmounting between engine changes (primarily meta engine->source engine navigation), it's only clearing state, so the unmount hook does not get fired. You do need to very explicitly call a specific action in useEffect
's return. If you skip this call you end up with intervals not properly getting cleared when switching engines.
if the Engine is empty, wait x second and call initializeEngine again
Going to strongly push back against using timeouts over setting an interval. We've done this in the past and it has previously lead to duplicate API calls - we had a discussion in Slack a while back about it, and most of the frontend devs who replied were in favor of moving away from setTimeout
for polling to setInterval
. In additional, it's a significantly clearer from an API signal standpoint that a poll is happening when you use setInterval
over setTimeout
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah. We ran into that issue before. Marius's recommendation was to key the logic and use BindLogic so that it gets reset when the engine changes.
Anyway, this still looks like it could be simplified to me, but you're in this deeper than me though, I'm not familiar with all of the guts and history, so if you think this is the best path then I'm fine with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh that would definitely solve that issue & I'd super be down to key EngineLogic sometime! Probably going to punt on it for 7.14 FF for the sake of time if that's cool (currently focusing on UI/UX polish over strictly dev/tech debt only items). I've made a backlog item though: https://github.com/elastic/app-search-team/issues/1853
I think keying actually lets us refactor out the "is engine stale" check also which is super cool. Definitely wanna do this for 7.15 😎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally! Also, wanted to say I really like the way you implemented it. Putting it in the EngineLogic is dope, we can take advantage of that in like every view without actually doing anything. Super elegant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, but I feel like there could be potential to simplify a bit. Left a suggestion, let me know what you think.
...gins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts
Outdated
Show resolved
Hide resolved
...gins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts
Show resolved
Hide resolved
...gins/enterprise_search/public/applications/app_search/components/engine/engine_logic.test.ts
Show resolved
Hide resolved
Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com>
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @JasonStoltz |
…3041) * Set up isEngineEmpty and isEngineSchemaEmpty selectors + update selector tests with mock engines to just an `engine` var (I saw Jason do this down below for `searchKey` and liked it, so I copied it) + update Documents & Search UI pages to new selectors * Update EngineOverview to use isEngineEmpty + remove polling - Per Casey, polling is primarily only needed to dynamically update from empty state to non-empty state, which we're going to handle at the engine level in the next commit * Add empty engine polling behavior - Now that both Engines Overview, Documents, and Search UI views have empty states that are hooked up to EngineLogic, this poll will update all of the above pages automatically when a new document comes in! * [Misc UI polish] Add (+) icon to Index documents button - to match other create page header buttons * [PR feedback] Test improvements Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…103205) * Set up isEngineEmpty and isEngineSchemaEmpty selectors + update selector tests with mock engines to just an `engine` var (I saw Jason do this down below for `searchKey` and liked it, so I copied it) + update Documents & Search UI pages to new selectors * Update EngineOverview to use isEngineEmpty + remove polling - Per Casey, polling is primarily only needed to dynamically update from empty state to non-empty state, which we're going to handle at the engine level in the next commit * Add empty engine polling behavior - Now that both Engines Overview, Documents, and Search UI views have empty states that are hooked up to EngineLogic, this poll will update all of the above pages automatically when a new document comes in! * [Misc UI polish] Add (+) icon to Index documents button - to match other create page header buttons * [PR feedback] Test improvements Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com> Co-authored-by: Constance <constancecchen@users.noreply.github.com> Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com>
Summary
This PR accomplishes several things:
Engine setup
orEngine overview
loads in immediately for a snappier feel.I strongly recommend following along by commit and checking out the commit messages.
Screencaps
Engine Overview:
Documents:
Search UI:
Checklist