Skip to content

[PIT finder] return empty generator if first page is empty#171598

Merged
pgayvallet merged 8 commits intoelastic:mainfrom
pgayvallet:kbn-xxx-PIT-finder-empty-page
Nov 27, 2023
Merged

[PIT finder] return empty generator if first page is empty#171598
pgayvallet merged 8 commits intoelastic:mainfrom
pgayvallet:kbn-xxx-PIT-finder-empty-page

Conversation

@pgayvallet
Copy link
Contributor

Summary

Makes the PIT finder more consistent by ignoring empty first page and not yielding it (as this is also what is done for other pages)

@pgayvallet pgayvallet added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// release_note:skip Skip the PR/issue when compiling release notes v8.12.0 labels Nov 21, 2023
@pgayvallet
Copy link
Contributor Author

@elasticmachine merge upstream

@pgayvallet pgayvallet closed this Nov 22, 2023
@pgayvallet pgayvallet reopened this Nov 24, 2023
@pgayvallet
Copy link
Contributor Author

@elasticmachine merge upstream

Comment on lines +96 to +100
// do not yield first page if empty, unless there are aggregations
// (in which case we always want to return at least one page)
if (lastResultsCount > 0 || this.#findOptions.aggs) {
yield results;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if this is an edge case, it is possible to specify aggregations with a PIT search, and this is effectively done at least once in our code:

let aggResults: UninstallTokenSOAggregationBucket[] = [];
for await (const result of idFinder.find()) {
if (
!result?.aggregations?.by_policy_id.buckets ||
!Array.isArray(result?.aggregations?.by_policy_id.buckets)
) {
break;
}
aggResults = result.aggregations.by_policy_id.buckets;
break;
}

Note that this specific usage could be adapted (given there isn't any reason to use a PIT search here), but we scenario remains valid more globally, so we still have to check for aggregations and yield, otherwise there's no way for the API consumers to retrieve their aggregation results in case of empty hits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @elastic/fleet as you may want to adapt this snippet to use a standard SOR.find call instead of a PIT given you break on first page.

@kibana-ci
Copy link

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@pgayvallet pgayvallet marked this pull request as ready for review November 27, 2023 10:22
@pgayvallet pgayvallet requested a review from a team as a code owner November 27, 2023 10:22
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

LGTM!

@pgayvallet pgayvallet merged commit 414260d into elastic:main Nov 27, 2023
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v8.12.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants