BewareMyPower commented on PR #23433: URL: https://github.com/apache/pulsar/pull/23433#issuecomment-2405113319
I fixed the test. And I also found the process on `NotFoundException` is unnecessary. <img width="1604" alt="image" src="https://github.com/user-attachments/assets/23946778-208b-468b-ad44-7da683843240";> The `testCloseAfterLoadingBundles` could still succeed even if we don't skip the orphan cleanup. So I kept the original behavior that when `doCleanup(broker, false)` is called and the broker is not healthy, continue the orphan cleanup. ---- However, from the failure of `testOverrideOrphanStateData`, I found `ServiceUnitStateChannelTest` is really confusing. In this test, `disableChannels` is called before all `overrideTableViews` calls. However, the `overrideTableViews` method is very complicated that it accesses the private fields via reflection and some operations will be performed on these fields. I guess this method is to simulate the `ServiceUnitStateChannel#overrideOwnership` method. But if there is a regression introduced to `overrideOwnership`, the existing test won't be able to detect the regression. Besides, the `disable()` method, which set the channel state to `Disabled`, was never called other than tests before https://github.com/apache/pulsar/pull/23349. After https://github.com/apache/pulsar/pull/23349, this method is only called in `cleanOwnerships()` and `doCleanup(broker, true)` will always be called after that. Should we still allow `doCleanup(broker, false)` scheduled by `handleBrokerDeletionEvent` be called after `disable()`? Anyway, these details are complicated and it could be a new topic to discuss. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
