jbertram commented on PR #5460: URL: https://github.com/apache/activemq-artemis/pull/5460#issuecomment-2605860614
First off, thanks for sending a PR! There's some good work here, but I have a few concerns. 1. There are a number of changes that aren't strictly required to fix the issue. For example, just in the first commit there's a block of code that is refactored into its own method as well as changes to a JavaDoc comment. Other commits change the names of variables, finalize variables, change the format of boolean expressions, etc. These unnecessary changes make it difficult to see where the actual semantic changes are. 2. At the very least, the first commit doesn't build properly because it contains a Checkstyle problem. 3. Code is added in one commit and then removed in another commit. For example, 4. Conceptually speaking this change violates the auth caching model used by the broker. Instead of having all the `Subject` values centralized in a single place (e.g. the cache) they also exist in the MQTT session state object. This increases the size of the heap makes the code more complex. It also subverts the expectation that auth entries will expire and be renewed. 5. Typically a PR is _single_ commit which includes all necessary code changes, tests, and doc updates. This simplifies the process of tracking changes for a specific bug or feature. You have 11 commits here which are all very directly related and could probably be squashed down to just 1. If the commits contains multiple notable changes then simply enumerate them in the commit message (e.g. like [this](https://github.com/apache/activemq-artemis/commit/10adca5479c7d0aa518c0ffce44008e95a268287)). Once you remove all the unnecessary changes that will simplify the commit substantially. -- 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: gitbox-unsubscr...@activemq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org For additional commands, e-mail: gitbox-h...@activemq.apache.org For further information, visit: https://activemq.apache.org/contact
