-
Notifications
You must be signed in to change notification settings - Fork 234
chore(indexes): remove in progress only when a matching index shows up in real indexes list; cleanup rolling on a timeout #6331
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
Conversation
…p in real indexes list; cleanup rolling on a timeout
3c4b6cd
to
7496180
Compare
indexId: inProgressIndexId, | ||
}); | ||
}, POLLING_INTERVAL * 3); | ||
} |
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.
how long does it normally take for the rolling index to be created? 15s doesn't sound like much
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 a bit hard to wrap the head around (tried to do it here but not sure if I did a good job to be honest), but we don't need to wait for the index to be created before this timeout check kicks in, we are only waiting for the rolling index build to start showing up in the list returned from the backend which corresponds to the index starting to be created on any of the nodes in the cluster, which happens way faster. As soon as it starts showing up in the list returned from the rolling indexes service, it will be removed from the in progress list and this action firing will be a no-op basically
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.
ah I see, thanks!
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.
Looks good! Are tests planned as a follow up?
|
||
// See action description for details | ||
if (isRollingIndexBuild) { | ||
setTimeout(() => { |
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.
Wondering if it is worth fussing over the timeout and clearing it. Probably not. Super rare to be adding rolling indexes and then there's only one and if the in-progress index disappeared in the meantime then it will just do nothing.
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.
That was my reasoning for not doing it, but I probably should put this in a comment, it's a rarely used feature and this action is harmless when it fires, so adding more code to store the reference seemed like an overkill
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.
Tests would be nice, but it is probably fine as is too.
@lerouxb @paula-stacho thanks for holding me accountable, will definitely add some tests 😄 |
Added tests, also drive-by refactored a bit the polling mechanism so that it can be cleaned up when plugin unmounts (writing a functional test for this started to make the tests "hang" until timeout resolves, so was easy to catch the missing cleanup issue) |
Based on the recent Slack discussion, we're adjusting the logic here a bit: