MINOR: Prevent this leak while DefaultStatePersister construction.#22292
Conversation
github-actions
Bot
added
triage
smjn
added
ci-approved
KIP-932
| private static final Logger log = LoggerFactory.getLogger(DefaultStatePersister.class); | ||
|
|
||
| public DefaultStatePersister(PersisterStateManager stateManager) { | ||
| public static DefaultStatePersister instance(PersisterStateManager stateManager) { |
There was a problem hiding this comment.
If we want DefaultStatePersister to manage the lifecycle of PersisterStateManager, should we let it instantiate the manager itself?
There was a problem hiding this comment.
@chia7712 Thanks for the feedback.
Incorporated change. Left some default visibility methods for ease of testing.
| } | ||
|
|
||
| // Visibility for tests | ||
| static DefaultStatePersister instance(PersisterStateManager stateManager) { |
There was a problem hiding this comment.
It appears all the passed stateManager instances are mocked. Maybe we could remove this helper and have the tests use the package-private constructor instead?
There was a problem hiding this comment.
@chia7712 Thanks for the suggestion, incorported.
| shareGroupTimer | ||
| ) | ||
| ) | ||
| klass.getDeclaredMethod("instance", classOf[KafkaClient], classOf[ShareCoordinatorMetadataCacheHelper], classOf[Time], classOf[Timer]) |
There was a problem hiding this comment.
Out of curiosity, why use a dynamic call here if we already know it is a DefaultStatePersister
There was a problem hiding this comment.
Hi,
initially the persister was planned to be a pluggable component and code was written in that way but it was subsequently decided to make it factory pluggable, hence the if condition. But yes with current state of code, this is unnecessary, will rectify
|
@chia7712 Thanks for the comments, incorporated. |
2 participants
DefaultStatePersisterinstantiatesPersisterStateManagerin its constructor. This implies that the
thiscreation is not yetcomplete and initializing another component.
factory method.
BrokerServercode has been updated to reflect the change.Reviewers: Chia-Ping Tsai chia7712@gmail.com