-
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
Handle another node already having deleted the temporary index #88332
Handle another node already having deleted the temporary index #88332
Conversation
Pinging @elastic/kibana-core (Team:Core) |
@elasticmachine merge upstream |
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! Just a couple of NITs regarding the bash script.
src/core/server/saved_objects/migrationsv2/integration_tests/parallel.sh
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/migrationsv2/integration_tests/parallel.sh
Outdated
Show resolved
Hide resolved
# kibana_index - search .kibana index against es | ||
# | ||
|
||
FN="$1" |
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.
optional: Not everyone knows bash as well as JS. What functionality not supported by JS?
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 think there's anything here that couldn't be implemented in JS, but I suspect it would just be a bit harder and more code to deal with async and streaming output between processes/files.
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.
Discussed offline, it would be good to have a long term goal of including multi-node upgrade migrations in automated tests. It would be much easier to implement such a test (and remove this script) once we have node clustering support #68626
rm processes.out | ||
for i in $(seq 0 $(expr $NUM - 1)) | ||
do | ||
PORT="56${i}1" |
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.
CI doesn't run the script. Does it?
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.
no, it's just for manual QA
@elasticmachine merge upstream |
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.
Since all the comments are based on the manual QA script test/scripts/run_multiple_kibana_nodes.sh
, but the actual code is correct, I'll 👍 this PR.
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
…ic#88332) * Handle another node already having deleted the temporary index * Make run_multiple_kibana_nodes.sh script more generic * Add note about dependency on jq Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
… (#88949) * Handle another node already having deleted the temporary index * Make run_multiple_kibana_nodes.sh script more generic * Add note about dependency on jq Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
Handles the temporary index already being deleted by another node as described in #84333
Related #75780
Checklist
Delete any items that are not applicable to this PR.
For maintainers