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

[App Search] Continue polling empty engines even when they have a schema #112915

Merged
merged 5 commits into from
Sep 27, 2021

Conversation

byronhulcher
Copy link
Contributor

@byronhulcher byronhulcher commented Sep 23, 2021

Summary

When we implemented empty engine polling in Kibana, we decided we would end the poll if the poll response indicated that the engine had any documents or any fields in its schema. This creates a negative experience for the Crawler. If an engine is created and immediately the user adds a domain and begins crawling it, the crawler process adds fields to the schema before it begins uploading documents. This will cancel the poll, and views such as EngineOverview and Documents which have specific empty states when an engine has no documents won't leave their empty states until the page is refreshed.

After this change we will continue polling as long as there are no documents.

As a part of this, I have renamed two of our selectors to clarify their functionality going forward. I renamed EngineLogic.values.isEngineEmpty to hasNoDocuments and renamed EngineLogic.values.isEngineSchemaEmpty to hasEmptySchema. This lines these selector names up with hasSchemaConflicts, hasSchemaConflicts and hasUnconfirmedSchemaFields. I think our values of isMetaEngine and isSampleEngine are fine to leave as is because they refer to an immutable property of the engine. These changes are in separate commits if we decide to roll this change back.

Checklist

@byronhulcher byronhulcher added release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 v7.15.1 labels Sep 23, 2021
@byronhulcher byronhulcher force-pushed the poll-engine-with-schema branch from 12122d2 to 18cae24 Compare September 23, 2021 02:58
@byronhulcher byronhulcher marked this pull request as ready for review September 23, 2021 14:03
@byronhulcher byronhulcher requested review from a team and orhantoy September 23, 2021 14:03
Copy link
Contributor

@orhantoy orhantoy left a comment

Choose a reason for hiding this comment

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

LGTM 👍

The changes make sense when talking about the initial crawl but how will the experience be for subsequent crawls that could create new documents?

@byronhulcher
Copy link
Contributor Author

The changes make sense when talking about the initial crawl but how will the experience be for subsequent crawls that could create new documents?

@orhantoy Just to clarify, during our migration to Kibana, product made a specific decision to go with only polling for initial documents. So this PR restores expected behavior. I needed to clear my conscience after the demo haha

There's definitely nothing stopping us from improving that behavior! We could be polling all the time, in fact the logic here would be even simpler. Or we wanted to narrow the case we could do a constant poll only while a crawl is active, which would add some complexity instead to our existing logic instead. Our standards around how "reactive" our UX is without user action are kind of ambiguous, its definitely something we haven't prioritized on the UX side, little has changed since we were still in ruby templates.

I think:

  1. we should create a ticket and decide if we'd like to do polling while a crawl is active or all the time
  2. as a follow-up, discuss what kinds of things we think its important to be reactive about, so we can make sure we're prioritizing that correctly for future features

@byronhulcher byronhulcher enabled auto-merge (squash) September 27, 2021 14:53
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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
enterpriseSearch 1.3MB 1.3MB -29.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x
7.15

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 27, 2021
kibanamachine added a commit that referenced this pull request Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.15.1 v7.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants