REFACTOR: Startup state stores initialization #20749
Conversation
| metricsRecorder.init(metricsImpl(stateStoreContext), stateStoreContext.taskId()); | ||
| openDB(stateStoreContext.appConfigs(), stateStoreContext.stateDir()); | ||
| if (!open) { | ||
| preInit(stateStoreContext); |
There was a problem hiding this comment.
maybe it's not a valid concern, but it looks like this interface would be hard to explain and thus use correctly: there is a two stage init process. sometimes we call preInit(phase one) explicitly, but there are a lot of cases when the author of the store needs to call it themselves in the init(phase two).
as we rely on stateStore.isOpen be true, maybe we should communicate that and follow the same path every time we want to initialize a store?
i.e.
- phase one(preInit/open)
- check if opened
- phase two(init)
it will slightly complicate the implementation on our side, but will make it significantly easier to follow for users
There was a problem hiding this comment.
Thank you for your time reviewing this PR.
That is a valid concern.
As things are right now in this PR, it's not clear when each method in the state store lifecycle is called. This makes things a lot harder for the people who write state stores, including the existing RocksDBStore.
I agree that we need to make the implementation a little harder on our end and a little easier on the specific state stores’ end.
I was thinking that we could document the preInit method and specify that it will be called immediately after the state store is built (in the topology builder), allowing state stores to implement logic for opening resources that will be required during the init phase. In the case of the RocksDBStore, we will open the store during this phase.
Although I am not familiar with custom state stores, I would assume that they could also open a database connection during this phase.
Since the state store was already opened during the preInit phase, the init method can only concentrate on preparing it for use in StreamThread processing (i.e., registering metrics in the process context). This is because the init method will run during the rebalance process.
State store authors would find it easy to implement or evolve their custom implementations with a more clear lifecycle.
| // TODO: we need to pass a proper logPrefix | ||
| StateManagerUtil.registerStateStores(log, "", subTopology, stateManager, this, initContext); | ||
| for (final StateStore stateStore : subTopology.stateStores()) { | ||
| if (!stateStore.isOpen()) { |
There was a problem hiding this comment.
I assume, the rest of stores will be updated later? right now there is only one Store updated: RockDBStore. I opened a random store(InMemoryKeyValueStore) and it sets the open field to true in the init method, so it will start failing here, I think
There was a problem hiding this comment.
Yes, this needs to be updated for the other stores.
2 participants
This pull request suggests adding a new stage for the state store lifecycle.
Since we don't open and/or initialize temporary tasks, we can fix the issue described in this issue: KAFKA-19434 by modifying the state store refactor. It is now possible to pre-initialize state stores and make them readable, which also is required by KIP-1035 implementation.