Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adapt es-archiver to call
_migrate
endpoint instead of creating a migrator #56971Adapt es-archiver to call
_migrate
endpoint instead of creating a migrator #56971Changes from 7 commits
1943e3a
b3d6019
b8820a0
110187e
ec1a4ab
d72d774
41f56b3
250ff7c
72cdb3d
6f02890
1f8b861
39b07f7
a793695
06f2138
4038539
c354dc7
f62f9ae
b467cfa
2217872
89ab46e
0ea85e0
801189a
10b8fac
96b1315
26a15e5
a79485d
318fa14
4fdb39d
502c042
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
TIL we need to manually generate and commit this file, and best part is it got mixed LF and CRLF line feeds.
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.
Can we add a
@param
to the comment to explain what this option does / how / when it should be used?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 change is very naive: if a migration is already running and we call
runMigration
again withreturn=true
, multiple migrations can occur at the same time.We should maybe wait for the pending migration to finish before triggering the next one. But that adds some complex logic to handle edge cases such as multiple
rerun=true
calls when a migration is already running. We should wait for the migration to finish but only re-execute ONCE and return the same promise for every calls torerun
that occurred during the initial migration.As this API is internal I though current implementation was acceptable. Tell me if we should fireproof it 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.
so, in theory, it could lead to a race condition if 2 migrations run over the same data?
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.
In theory it could yes, even if I don't think it could leads to data corruption, as in worse case, both 'threads' will access the data before any actually migrated it, and the document would be migrated then updated 'twice' from the same original data, which would only impact performances.
I can try to improve that by 'pooling' all the calls to
runMigration(rerun=true)
while one migration is running and only execute the rerun once, as explained in previous comment, if we think current implementation is too fragile.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.
It's okay as long as this API is used only for tests internally. Could you add a comment about this known limitation?
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.
Could we log a warning if this endpoint is used in production env? (and maybe if the CI env var isn't set?)
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.
Also there aren't any tests on this endpoint / logic. I know this is an internal API, but it still feels like we should test that we don't break this by accident. Though CI would probably fail if this broke, it would be much better if we had more insight into what broke to cause the failure.
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.
We are running the FTR suites against production cloud, so I'm not sure we should be logging a warning? At least we should not check the CI var env?
Yea, I know. I can add basic unit test to check that 1/ the route is registered and 2/ calling the route properly calls the migrator. However I have no idea how to go further than that, as the API call is actually testing in the FTR. We can't really functional test a component used by the FTR framework itself, can we? Any insight on how to test that on a 'higher' level that unit is welcome.
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 added integration tests on the endpoint to ensure it properly calls
migrator.runMigrations
with the correct parametersThere 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.
Is Platform going to promote any naming convention to clearly distinguish public (with BWC guarantees and documentation) and internal (no BWC guarantees, implicit
use at your own risk
warning and optional documentation) API endpoints? In Spaces/Security we started to define internal routes with/internal/
prefix instead of/api/
as an experiment that has been working well enough so far.I'm a bit concerned that users will quickly discover this endpoint and may start using it in any imaginable context without realizing all the consequences leading to hard-to-handle/diagnose support cases....
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.
Anyone against
/internal/saved_objects/_migrate
?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 don't want to introduce another convention without a broad discussion. as of me,
internal
could work, yes.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.
By the way, does it mean that
env.packageInfo
(dist
,buildNum
etc.) in this case is completely indistinguishable from the one we have when Kibana is run in production outside of functional tests?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.
/internal/saved_objects/_migrate
kinda breaks the SO route prefix. This is very minor, but forces to use a/
based router when declaring routes instead of a/saved_objects
one. Would/saved_objects/internal/_migrate
be acceptable, or do we really want the route to starts with/internal
?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.
nevermind, did not understand the
internal
prefix was a replacement for theapi
one. Please forget my previous questionThere 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.
question: is
result
going to be consumed by the client somehow? If not, maybe it'd be safer to not return it at all for now (ideal option, we can log though) or add type annotation here (so that TS complain if signature changes) or just pick specific fields in case migrator starts returning something more sensitive? I know it sounds a bit paranoid, but you never know.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.
Not atm. I can just return a
{status: 'ok'}
dummy response insteadThere 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.
Registration is done during
setup
, the migrator is not yet created. Had to use a subject/promise as workaround.