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
Resolve #1211 | Read only mode persistence #1214
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.
🎉
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.
Reading the test "should change status from READ_ONLY_ADMIN to READ_WRITE only by admin" I came to the conclusion that the api exposed by ModeService
is not obvious. Let's suppose we have to add another component responsible for changing the mode. It will be not obvious why should I call compareAndSwapMode
instead of setMode
. It will be necessary to read the implementation and tests to understand the consequences of using setMode
. So I suggest to change these methods:
setMode
->setModeByAdmin(expectedMode)
- set modecompareAndSwapMode
->setMode(expectedMode)
- set mode if current mode is not equal toREAD_ONLY_ADMIN
.
What do you think?
Anyway, I tested this PR locally with 2 ZK clusters and it works correctly.
...t/src/test/groovy/pl/allegro/tech/hermes/management/domain/health/HealthCheckTaskTest.groovy
Outdated
Show resolved
Hide resolved
...t/src/test/groovy/pl/allegro/tech/hermes/management/domain/health/HealthCheckTaskTest.groovy
Outdated
Show resolved
Hide resolved
…dOnlyAdmin flag as well
002db3d
* Resolve allegro#1211 | Read only admin flag added to Mode Service * Resolve allegro#1211 | Tests added for READ_ONLY_ADMIN flag * Resolve allegro#1211 | Thread-safe version * Resolve allegro#1211 | Changes after code review; admin endpoint accepts ReadOnlyAdmin flag as well
Read only mode set by admin (READ_ONLY_ADMIN) in hermes management is not overwritten by healthcheck task.