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

Jump to bottom

Fix SO export sorting algorithm #142078

Merged
merged 5 commits into from
Sep 29, 2022
Merged

Fix SO export sorting algorithm #142078

merged 5 commits into from
Sep 29, 2022

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Sep 28, 2022 •
edited by kibanamachine
Loading

Summary

Fix #142055

Release note

Fix a bug causing the savedObjects 'export' and 'copy to space' features to sometimes cause Kibana to crash for very complex graphs of objects.

@pgayvallet pgayvallet added bug Fixes for quality problems that affect the customer experience Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:fix backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) backport:prev-major Backport to (8.x, 8.17, 8.16) the previous major branch and other branches in development v8.6.0 labels Sep 28, 2022
pgayvallet
pgayvallet commented Sep 28, 2022
View reviewed changes
...re/saved-objects/core-saved-objects-import-export-server-internal/src/export/sort_objects.ts
Comment on lines +31 to +33
traversed.add(objectId);
if (objectRefs.length) {
includeObjects(objectRefs);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the significant change of the PR, the rest of the diff in this file is just minor enhancement/cleanup.

Previously, we were removing the current object from the 'path' when we're done traversing the children, causing the algo to eventually traverse the whole tree again if the same object is encountered in the tree of another node from a higher level.

Once a object has been traversed once, we now keep it in the path list (renamed to traversed as it better reflect its role now), avoiding the full traversal again later.

...ved-objects/core-saved-objects-import-export-server-internal/src/export/sort_objects.test.ts
Comment on lines +431 to +433
test('should not fail on large graph of objects', () => {
// create an object that references all objects with a higher `index` up to `depth`.
const createComplexNode = (index: number, depth: number): SavedObject => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried it without the fix, and the test does timeout.

pgayvallet
pgayvallet commented Sep 28, 2022
View reviewed changes
...s/core-saved-objects-import-export-server-internal/src/export/saved_objects_exporter.test.ts
Comment on lines 251 to +252
const firstMockHits = generateHits(1000, { sort: ['a', 'b'] });
const secondMockHits = generateHits(500);
const secondMockHits = generateHits(500, { idPrefix: 'second-hit-' });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was generating multiple pages of SOs with duplicate ids. The fix performed in this PR also has the impact of removing duplicates from top level / initial objects. Note that this will never occur in production given we are removing duplicates from the top level exported objects anyway.

@pgayvallet pgayvallet marked this pull request as ready for review September 29, 2022 05:47
@pgayvallet pgayvallet requested a review from a team as a code owner September 29, 2022 05:47
@elasticmachine
Copy link
Contributor

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

rudolf
rudolf approved these changes Sep 29, 2022
View reviewed changes
Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

🎉

@pgayvallet
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@pgayvallet pgayvallet merged commit 4cdd74d into elastic:main Sep 29, 2022
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 29, 2022
@pgayvallet
Fix SO export sorting algorithm (elastic#142078)
2dbc144 
* Fix SO export sorting algorithm

* improve var name

* adapt another unit  test

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit 4cdd74d)
This was referenced Sep 29, 2022
Merged
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 29, 2022
@pgayvallet
Fix SO export sorting algorithm (elastic#142078)
77c8db2 
* Fix SO export sorting algorithm

* improve var name

* adapt another unit  test

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit 4cdd74d)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 7.17
✅ 8.5

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Sep 29, 2022
@kibanamachine @pgayvallet
Fix SO export sorting algorithm (#142078) (#142257)
1e88196 
* Fix SO export sorting algorithm

* improve var name

* adapt another unit  test

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit 4cdd74d)

Co-authored-by: Pierre Gayvallet <[email protected]>
@kibanamachine kibanamachine added v8.5.0 backport missing Added to PRs automatically when the are determined to be missing a backport. labels Sep 29, 2022
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 2, 2022
@pgayvallet @kibanamachine @WafaaNasr
Fix SO export sorting algorithm (elastic#142078)
9ece72f 
* Fix SO export sorting algorithm

* improve var name

* adapt another unit  test

Co-authored-by: Kibana Machine <[email protected]>
kibanamachine added a commit that referenced this pull request Oct 3, 2022
@kibanamachine @pgayvallet
[7.17] Fix SO export sorting algorithm (#142078) (#142256)
8a52086 
* Fix SO export sorting algorithm (#142078)

* Fix SO export sorting algorithm

* improve var name

* adapt another unit  test

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit 4cdd74d)

* fix import for 7.x

Co-authored-by: Pierre Gayvallet <[email protected]>
@kibanamachine kibanamachine added v7.17.7 and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rudolf rudolf rudolf approved these changes

Assignees
No one assigned
Labels
backport:prev-major Backport to (8.x, 8.17, 8.16) the previous major branch and other branches in development backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) bug Fixes for quality problems that affect the customer experience release_note:fix Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.17.7 v8.5.0 v8.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SO export can OOM during sorting step for complex graphs of objects
5 participants
@pgayvallet @elasticmachine @kibana-ci @kibanamachine @rudolf