jvz commented on PR #3869: URL: https://github.com/apache/logging-log4j2/pull/3869#issuecomment-3151513239
> * The methods `ReusableMessage.memento()` and `LogEvent.toMemento()` have similar names but return fundamentally different types (`Message` vs. `LogEvent`). This naming overlap can cause confusion, especially since classes like `ReusableLogEvent` implement both interfaces (`ReusableMessage` and `LogEvent`). The naming overlap with similar but not equal method names is related to what you identified here (classes implementing both `Message` and `LogEvent`). Perhaps it would make more sense for the methods to be renamed to `toMementoMessage()` and `toMementoEvent()`. In case it wasn't clear by the names, I used the term "memento" here as a design pattern name for a snapshot of some object. As the old implementation made somewhat more explicit, it's the logical equivalent of running a data class through some form of round trip serialization/deserialization. While a memento object might be modifiable thanks to the type system, the point of a memento object is that such accidental manipulation doesn't affect the original object. In general, though, this problem would be easier to denote in the type system if Java supported immutable types like Kotlin does. 🤷🏼 > * Introducing a method such as `freeze()`/`isFrozen()` could be beneficial in scenarios where the log event has reached its final state and is guaranteed not to change further. That sounds a little bit of an internal detail considering these checks are largely relevant to when an object is returned a pool (which is how we got rid of the frozen flag in some reusable class after adding `Recycler`). > * Considering that approximately 99% of log messages utilize the standard three types (`ObjectMessage`, `SimpleMessage`, and `ParameterizedMessage`), it may be advantageous to streamline the logging pipeline by exclusively using either `Log4jLogEvent` or `MutableLogEvent`. These classes could implement both the `Message` (or `ReusableMessage`) and `LogEvent` interfaces, thereby simplifying the architecture. I think with the changes you made such that `Log4jLogEvent` is effectively immutable while `Log4jLogEvent.Builder` is mutable sort of works toward that idea. While I haven't verified the feasibility of it yet, perhaps `MutableLogEvent` can be replaced by `Log4jLogEvent.Builder`. > These are preliminary thoughts, and since I haven't reviewed the changes thoroughly yet, some of these ideas might already be implemented. I did remove an extra `LogEvent` implementation thanks to the changes from 2.x. Perhaps there is more simplification possible with `MutableLogEvent`. -- 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]
