Unit testable IndicesClusterStateService#17270
Conversation
There was a problem hiding this comment.
This comment is stale now (no longer package private).
8ac5491 to
45c0b6f
Compare
ae80607 to
f71f7de
Compare
There was a problem hiding this comment.
s/to list of/to the list of/
|
@ywelsch this looks AWESOME! I really like it. I think we should get it in ASAP! I left some bikeshedding while I looked at the structure, I wasn't sure if you wanna go over it again before we do a more detailed review! IMO the tests by itself justify pushing this soon :) |
There was a problem hiding this comment.
wondering if we need recoveryState.isPeerRecovery() to simplify these lines.
There was a problem hiding this comment.
I've switched the flow here to switch / case so that it's apparent that we cover all cases.
There was a problem hiding this comment.
I think requiresIndexMappingRefresh is very misleading - it should be called updateMapping as it does update the mapping (and returns a boolean for refreshes)
There was a problem hiding this comment.
should we make this an impl detail on AllocatedIndex#updateMetaData and let it return if we need to refresh the mapping?
There was a problem hiding this comment.
++. PS - We shouldn't optimize for refresh mapping as it's going away.
There was a problem hiding this comment.
We currently call updateIndexMapping explicitly when we create an index (the mapping is not updated as part of index creation, that's a whole different rabbit hole to investigate) but don't call updateMetaData on the newly created index as meta data is properly initialized by createIndex. This means that I cannot just fold updateMetaData and updateMapping together. I would prefer not to address this point in this PR.
There was a problem hiding this comment.
Sure. This one is more than good enough :)
This PR makes IndicesClusterState unit-testable. Testability of ICSS is achieved by introducing interfaces for IndicesService, IndexService and IndexShard. These interfaces extract all relevant methods used by ICSS (which do not deal directly with store). This gives the possibility to easily mock all the store behavior away in the tests (and cuts down on dependencies). It also helps to better understand what the actual interface is between the different components.