Thanks Jian He for the feedback. Updated patch (v2) to remove redundant null check and refactor setStateStoreClient as suggested by you.
As to your other questions:
we already have RM_CLUSTER_ID, any chance that this can be used for FEDERATION_SUBCLUSTER_ID ?
That's a possibility. The reason I didn't combine both is RM_CLUSTER_ID is currently used for HA but Federation can work both with and without HA (and RM HA can work both with and without Federation). So felt it would be better to keep them separate. Thoughts?
I feel the SubClusterState is a bit redundant in the request object, because the API itself already indicates the state such as register / deregister.
You are right. We don't want state to be null in the store so either the store impl can implicitly add SC_NEW/SC_UNREGISTERED on register / deregister or the invoker (which is always RM) can. I decided to do it in the RM for 2 reasons:
1. It is trivial (one line) & needs to be done in a single place (RM) instead of in each store impl we add.
2. This allows for flexibility future as RM could potentially register / deregister with different states (say SC_DRAINING).