Refactor MoveDecision method names for clarity #136139
Conversation
DiannaHohensee
added
>non-issue
:Distributed Coordination/Allocation
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
|
@nicktindall I was eyeing these old names when reviewing your shard selection PR. Hopefully this change makes it easier to understand the call-site code. |
| public boolean canRemain() { | ||
| public boolean canRemainYes() { |
There was a problem hiding this comment.
This change does not seem necessary to me. The Yes part is quite obviously implied for a boolean method.
There was a problem hiding this comment.
I got tripped up by the name, so if it doesn't cause any harm, it'd be nice to make the purpose extra clear.
The fact that the method returns a boolean, and not a canRemain decision, was one point of confusion that this change hopefully clarifies. The other is that there are more decisions than NO and YES these days (THROTTLE, NOT_PREFERRED), so the change further clarifies why the shard should/can stay.
There was a problem hiding this comment.
I'm also less excited about this change, it seems to break encapsulation for me. For whatever reason we have a MoveDecision and it boils canRemain down to a boolean. The canRemainDecision seems like an implementation detail? and one the caller shouldn't care about?
There was a problem hiding this comment.
... it boils canRemain down to a boolean. ... and one the caller shouldn't care about?
I'm not convinced these are good arguments for the status quo, since the interface is old and inconsistent and could easily be handing back a number of types for canRemain other than a boolean (and what does the boolean mean?), but I'll change it back.
| */ | ||
| public static MoveDecision remain(Decision canRemainDecision) { | ||
| public static MoveDecision createMoveDecisionWithRemainYesDecision(Decision canRemainDecision) { | ||
| assert canRemainDecision.type() != Type.NO; |
There was a problem hiding this comment.
Does canRemain ever return THROTTLE, does that even make sense? I wonder why we aren't more specific in this method?
There was a problem hiding this comment.
This is 10 year old code, maybe older. It hasn't been updated in a very long time. I tracked it back to the BalancedShardsAllocator (many refactors), but then gave up :) THROTTLE was added in 2012. This is original Elasticsearch founder code -- i.e. completed quickly, not prettily.
There was a problem hiding this comment.
Can we also assert canRemainDecision.type() != Type.NOT_PREFERRED? Otherwise the method name would be inaccurate.
| public boolean cannotRemainAndCanMove() { | ||
| checkDecisionState(); | ||
| return canRemainYes() == false && canMoveDecision == AllocationDecision.YES; | ||
| } |
There was a problem hiding this comment.
Good call, forceMove also irked me the whole time I was working on that. Forgot to come back to it though.
|
Changed canRemainYes back to canRemain. Ready for another look 👍 |
| */ | ||
| public static MoveDecision remain(Decision canRemainDecision) { | ||
| public static MoveDecision createMoveDecisionWithRemainYesDecision(Decision canRemainDecision) { | ||
| assert canRemainDecision.type() != Type.NO; |
There was a problem hiding this comment.
Can we also assert canRemainDecision.type() != Type.NOT_PREFERRED? Otherwise the method name would be inaccurate.
| * be forced to move to another node. | ||
| */ | ||
| public static MoveDecision remain(Decision canRemainDecision) { | ||
| public static MoveDecision createMoveDecisionWithRemainYesDecision(Decision canRemainDecision) { |
There was a problem hiding this comment.
Nit: I think such a static method name usually skip the type name, e.g.:
| public static MoveDecision createMoveDecisionWithRemainYesDecision(Decision canRemainDecision) { | |
| public static MoveDecision withRemainYesDecision(Decision canRemainDecision) { |
| */ | ||
| public boolean cannotRemainAndCannotMove() { | ||
| checkDecisionState(); | ||
| return canRemainDecision.type() != Type.YES && canMoveDecision != AllocationDecision.YES; |
There was a problem hiding this comment.
Nit: can we have the check consistent with cannotRemainAndCanMove so that either both use canRemain() == false or canRemainDecision.type() != Type.YES?
| return canRemainDecision.type() != Type.YES && canMoveDecision != AllocationDecision.YES; | |
| return canRemain() == false && canMoveDecision != AllocationDecision.YES; |
4 participants
These method names reflect the purpose for which they were used in
the past, rather than what they mean. Given how much the code has
changed since class/methods were created, and the different ways in
which they are now used, the names have become confusing. This commit
changes some names to better reflect meaning.