mon: remove_is_write_ready() #19191
mon: remove_is_write_ready() #19191
Conversation
tchaikov
force-pushed
the
wip-mon-cleanup
branch
from
111fe63
to
93fcd34
Compare
src/mon/AuthMonitor.cc
Outdated
| @@ -275,7 +275,7 @@ version_t AuthMonitor::get_trim_to() const | |||
| { | |||
| unsigned max = g_conf->paxos_max_join_drift * 2; | |||
| version_t version = get_last_committed(); | |||
| if (mon->is_leader() && (version > max)) | |||
| if (version > max) | |||
There was a problem hiding this comment.
Why would we remove an is_leader() check?
I get that it's redundant with an earlier check. But nonetheless, we can only trim if we are a leader. It's redundant in the sequence we currently call it, but it's not incorrect. And those sorts of safeties make the code much more resilient to future changes.
There was a problem hiding this comment.
okay. i thought is_writable() implies is_leader(). but if it's just a side-effect of how we update have_pending, and hence it is not necessarily a post-condition of if (is_writable()), the change is wrong.
on a second thought, can we instead assert(is_leader()) in get_trim_to()? does this help with the safeties?
There was a problem hiding this comment.
is_writeable() implies is_leader(), but in this case we're doing a check for local requirements and it just so happens a previous caller also checked it.
This isn't an expensive check so it's not worth the cognitive load of future developers to have to check that condition in all callers. (We could, for instance, have trimming be run locally on each daemon rather than be an operation organized by the paxos leader.)
There was a problem hiding this comment.
i am not convinced that a function like this is supposed to be very defensive and can be called without a pre-conditions of is_leader(): that where the "undefined behavior" kicks in and assert(is_leader()) helps the future developer to understand this precondition. yes, we could, but like premature-optimization, i'd name it over-engineering for a non-public API..
also, please note that not all get_trim_to()s do this check. that's why i started to do this clean up. IMO, the inconsistency is a root of confusion and probably bug. anyway, like you mentioned, this is not an expensive check. i will drop this commit.
is_writeable() is an alias of is_write_ready(). the former is a public interface, and is well documented. so let's keep is_writeable(). Signed-off-by: Kefu Chai <kchai@redhat.com>
tchaikov
force-pushed
the
wip-mon-cleanup
branch
from
93fcd34
to
76f77b3
Compare
tchaikov
changed the title
|
@gregsfortytwo ping? |
No description provided.