-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Improve es-archiver saving/loading #118255
Conversation
💚 CLA has been signed |
9fd6498
to
e026218
Compare
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.
Author's notes for reviewers.
It looks like the kbn-es-archiver package has been changed several times over the years to support various edge cases. I aimed to make minimal changes to this so that it will continue to behave the same way for existing usage, but we have the option to modify its behavior for more complex saving/loading.
@@ -75,9 +78,9 @@ function isKibanaIndex(index?: string): index is string { | |||
); |
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 didn't change this code so I can't select the whole block, but it looks like the isKibanaIndex
function doesn't match up with the TS doc comment.
The comment says "(e.g. we don't want to remove .kibana_task_manager or the like)", but it looks like the check was later modified to match the .kibana_task_manager
index anyway.
At any rate, the change I made modifies the index pattern that is fetched, and isKibanaIndex
is only used later to filter the results, so this disparity doesn't really matter in practice. But I thought I'd point it out.
function createMockClient(responses: SearchResponses) { | ||
// TODO: replace with proper mocked client | ||
const client: any = { | ||
helpers: { | ||
scrollSearch: jest.fn(function* ({ index }) { |
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 abstracted this out into a reusable function, but it could/should be improved so I left a TODO here. I wanted to get this PR done with minimal changes so I opted not to change this existing mock logic.
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
To update your PR or re-run it, just comment with: |
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.
LGTM, thanks for the thuroughly working out how to improve the ES Archiver in this case. I assume since you're working with task manager stuff that you're working with Saved Objects which can't be imported/exported with the new kbnServer.importExport
API?
Sure thing!
Yeah, 1. some of the objects are not importable/exportable, 2. you can't trigger an index migration on objects that you import through that API. |
Looks like es-archiver functionality has diverged enough so that you can't use es-archiver from 8.0 to save documents from 7.16. |
* Improve es-archiver saving/loading (#118255) # Conflicts: # packages/kbn-es-archiver/src/actions/load.ts # packages/kbn-es-archiver/src/lib/indices/create_index_stream.ts # packages/kbn-es-archiver/src/lib/indices/generate_index_records_stream.ts # packages/kbn-es-archiver/src/lib/indices/kibana_index.ts * Fix failing unit test Accidentally deleted some lines when fixing merge conflicts, had to add them back again.
Resolves #118209.
This PR makes some changes to the es archiver:
save
command, which avoids renaming all.kibana*
indices to.kibana_1
.kibana_task_manager
index don't get added to the.kibana
index.load
command, which loads documents from an archive without changing any existing indices..kibana_task_manager
index alone won't delete other.kibana*
indices (currently, the only other one is the main.kibana
index)