-
Notifications
You must be signed in to change notification settings - Fork 843
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
[BTS-1644] ResignLeadership + Wait #21401
base: devel
Are you sure you want to change the base?
[BTS-1644] ResignLeadership + Wait #21401
Conversation
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've got two type-improvement suggestions, otherwise LGTM.
arangod/Agency/ResignLeadership.cpp
Outdated
<< "Not starting resign leadership job because some shards have no " | ||
"common in sync follower"; | ||
// check if a timeout value is specified | ||
if (_waitForInSyncTimeout > 0) { |
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.
Do I understand it correctly that if _waitForInSyncTimeout == 0
, there is no timeout and we can wait indefinitely? This is a bit confusing, because 0 timeout normally just means instantly.
Possible fix: We could have an optional instead of just a number and if the the snapshot-velocypack does not include waitForInSyncTimeout
, the option is std::nullopt
.
arangod/Agency/ResignLeadership.h
Outdated
@@ -55,6 +55,8 @@ struct ResignLeadership : public Job { | |||
|
|||
std::string _server; | |||
bool _undoMoves{true}; | |||
bool _waitForInSync{false}; | |||
uint64_t _waitForInSyncTimeout{30 * 60}; |
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.
30 minutes sound a lot to me, but perhaps this is correct.
This value is actually set per default to 0 in the ResignLeadership constructor, so I guess it is more readable to set it to zero here as well.
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.
Another thing: I would be nice to make it visible that they both belong together - _waitForInSyncTimeout
does not make any sense if _waitForInSync
is false - as far as I understood.
E.g. via an std::variant<bool, std::tuple<bool, TimeoutInSec>>
or with the comment above: std::variant<bool, std::tuple<bool, std::optional<TimeoutInSec>>>
…t-for-followers-resign-leadership
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Lars Maier.
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Lars Maier.
|
0bd0aa0
to
fd96027
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Lars Maier.
|
fd96027
to
4780410
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Lars Maier.
|
This is still relevant and I will finish it eventually, when no more pressing issues are there. |
…t-for-followers-resign-leadership
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Lars Maier.
|
Scope & Purpose
Added waitForInSync and waitForInSyncTimeout parameters to the resign leadership job. This allows the user to wait for a certain amount of time to make sure that there exist common in sync followers. Previously they were just ignored and could cause downtime.
Design Doc: https://github.com/arangodb/documents/pull/136