[Helix-656] Support customize batch state transition thread pool #91
Conversation
| return; | ||
| } | ||
|
|
||
| if (!_batchMessageThreadpoolChecked) { |
There was a problem hiding this comment.
It seems to me as long as there's any STATE_TRANSITION message, this pool will be created, so why not simply instantiate this thread pool in the constructor?
There was a problem hiding this comment.
There is no HelixManger or ZkClient to read configurable information from ZK in constructor.
There was a problem hiding this comment.
Would it be better to pass it in? Each HelixManager for sure has a DefaultMessagingService, which for sure has a HelixTaskExecutor. Using NotificationContext to pass in the "parent object" just to do initialization doesn't seem quite straightforward.
When I work on adding ThreadPoolExecutorMonitor for HelixTaskExecutor, I actually have the same problem of getting InstanceType and ClusterName from HelixManager. Maybe we should add it in the constructor?
There was a problem hiding this comment.
This is not a proper solution for the problem. We shall keep the backward compatible. HelixTaskExecutor has a default constructor now. In this case, if the HelixTaskExecutor instantiated by default constructor does not have any information for configurable threadpool. That's why there is also a logic to make the threadpool configurable in a separate method (not only for batch message thread pool). Otherwise, the user defined the information will be ignored. It is not good just remove the public constructor.
There was a problem hiding this comment.
I see, just to confirm, is backward compatibility the only concern here?
I'm wondering if constructing HelixTaskExecutor with a HelixManager would make more sense, at all, if we're free to change the constructor?
To better support batch message handling, we shall make batch state transition thread pool configurable.
One issue we observed is that when batch messages enabled, it will have NPE in ZNRecord merge record. Race condition could be the root cause. The only place can have race condition is the current state update map in NotificationContext, which is passed as input for multiple sub tasks in BatchMessageHandler.
4 participants
To better support batch message handling, we shall make batch state transition thread pool configurable. This config can be put in cluster config.