lhotari commented on issue #25660: URL: https://github.com/apache/pulsar/issues/25660#issuecomment-4629597212
> If this approach looks good to you, I can open follow-up PRs for the other modules (like client, connectors, etc.) in smaller batches. Let me know if you prefer a different strategy! I'm sorry that the issue description isn't clear. It's an observation that I made earlier. Some thoughts: It's better to first research and create a plan before going forward. As your draft PR already mentions in the description, the main reason for doing this is to be able to write tests that wouldn't be tied to the real wall clock and could use a mock clock that could be controlled by the test. In the research (that an AI agent could perform), it would be good to find out why the current code base contains this mixed usage of `java.time.Clock.millis()` and `System.currentTimeMillis()` and what tests currently rely on the Clock. In addition, the research should look into current time sensitive tests (message expiry?) that would benefit of having a consistent usage of `java.time.Clock` across the code base. One minor detail about the existing PR #25935. When locating the `Clock` instance, it's better to store the instance in a field instead of having a long chain of calls for each usage such as `subscription.getTopic().getBrokerService().getPulsar().getClock().millis()`. One of the benefits is that in unit tests, the class doesn't need to mock all levels to be able to inject a mock Clock instance. In the Pulsar code base, we haven't paid sufficient attention to have the code base structured as "units" which could be tested with unit tests. Most tests in Pulsar aren't really unit tests since they require a large amount of mocked dependencies or a complete mocked Pulsar broker to run. The problem that this causes is that boundaries of "units" fade out and in the code some parts become very coupled without a clear separation of concerns. In Pulsar code base, the [lack of clear definition of a concurrency model](https://github.com/apache/pulsar/blob/master/ARCHITECTURE.md#concurrency-model-a-known-gap) brings in yet another challenge. Summary: This issue is just a tip of the iceberg of improving testability and code quality in the Pulsar code base. We can first focus on the `java.time.Clock` research part. Would you like to perform that? You could also store the research results in https://gist.github.com/ as markdown with possible mermaid diagrams and reference that in a comment on this issue. (Gists can only hold text files. When updating the files in a gist, it's possible to checkout the gist as a git repository locally and push updates as git commits. ) -- 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]
