LogstashMarker refactoring #636
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Most Logstash marker don't have any children/reference at all. Creating the referenceList upfront is a waste of memory and additional pressure on the GC. This commit modifies the implementation to create the reference list when the first reference is added.
This class is now our own implementation - remove copy of SLF4J copyright notice
|
@philsttr |
philsttr
reviewed
Sep 9, 2021
There was a problem hiding this comment.
@philsttr EmptyLogstashMarker does not override equals/hascode. Equality is therefore solely on the marker name. Most (if not all) other logstash markers also require that "other" is of the exact same class. Shouldn't it be the case here as well ?
Yes, please add a check for the same class.
src/main/java/net/logstash/logback/marker/LogstashBasicMarker.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/marker/MapEntriesAppendingMarker.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/argument/DeferredStructuredArgument.java
Show resolved
Hide resolved
contains() should accept null values and return false in this case. Calling code will not have to test for nullability before using the method and still have a consistent result
Revert changes made to equals() in f29bb2c
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Most Logstash marker don't have any children/reference at all. Creating the referenceList upfront is a waste of memory and additional pressure on the GC. This PR modifies the implementation to create the reference list eagerly when the first reference is added. See #617 (comment) for the motivations behind this change.