[GitHub] kafka pull request #3904: [MINOR] Added equals() method to Stamped
Github user KoenDG closed the pull request at: https://github.com/apache/kafka/pull/3904 ---
[GitHub] kafka pull request #3891: [WIP input needed] MINOR: Further code cleanup inv...
Github user KoenDG closed the pull request at: https://github.com/apache/kafka/pull/3891 ---
[GitHub] kafka pull request #4226: MINOR: Arrays.toList replaced with Collections.sin...
GitHub user KoenDG opened a pull request:
https://github.com/apache/kafka/pull/4226
MINOR: Arrays.toList replaced with Collections.singletonList() where
possible.
This is something I did after my working hours, I would ask people
reviewing this do the same, don't take time for this during your work hours.
It's not actual functionality, it is an internal rewrite based on suggestions
provided by the static code analysis built into the Intellij IDE.
I try to keep such a PR as limited as possible, for clarity of reading.
==
In places where Arrays.asList() is given exactly 1 argument, replace it
with Collections.singletonList().
Internally, this is a smaller object, so it uses a bit less memory at
runtime.
An important thing to note is that you cannot add anything to such a list.
So if it is returned through a method into a random `List<>` object, performing
`add()` on it will throw an exception.
I checked every usage of all replaced instances, they're only ever returned
to be read, so this should not occur. A
One might say this is a micro-optimization, I would say that's true, it is.
It's up to the maintainers whether or not they want this in. Personally I
checked all code usages and did not find a point where an exception would be
caused because of these changes. So it should not break anything and provide a
tiny improvement in terms of memory footprint at runtime.
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/KoenDG/kafka singletonlist
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/kafka/pull/4226.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4226
commit 772d65f2411aac1f78b12cd4a52547a48fa8f9f4
Author: KoenDG
Date: 2017-11-16T19:42:54Z
In places where Arrays.asList() is given exactly 1 argument, replace it
with Collections.singletonList().
Internally, this is a smaller object, so it uses a bit less memory at
runtime.
An important thing to note is that you cannot add anything to such a list.
So if it is returned through a method into a random List<> object, performing
add() on it will throw an exception.
I checked every usage of all replaced instances, they're only ever returned
to be read, so this should not occur.
One might say this is a micro-optimization, I would say that's true, it is.
It's up to the maintainers whether or not they want this in. Personally I
checked all code usages and did not find a point where an exception would be
caused because of these changes. So it should not break anything and provide a
tiny improvement in terms of memory footprint at runtime.
Import changes due to Intellij squashing imports into a * import. Had to
update my settings so it won't happen again.
---
[GitHub] kafka pull request #3886: Code cleanup, subject: log statements.
GitHub user KoenDG opened a pull request:
https://github.com/apache/kafka/pull/3886
Code cleanup, subject: log statements.
I'm doing this in my spare time, so don't let reviewing this PR take away
actual work time. This is just me going over the code with the Intellij
analyzer and implementing the most easily implementable fixes.
This PR is focused only on seemingly erronous log statements.
1: A log statement that has 4 arguments supplied but only 3 `{}` statements
2: A log statement that checks is debug is enabled, but then logs on `info`
level.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/KoenDG/kafka loggingErrors
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/kafka/pull/3886.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3886
commit 2e95300a70fdf331224051de04fd9ee8cf1763cb
Author: coscale_kdegroot
Date: 2017-09-18T09:26:38Z
Code cleanup, subject: log statements.
---
[GitHub] kafka pull request #3891: [WIP] MINOR: Further code cleanup involving log st...
GitHub user KoenDG opened a pull request: https://github.com/apache/kafka/pull/3891 [WIP] MINOR: Further code cleanup involving log statements First of all, terribly sorry for not having included this in the previous PR, that one only focused on the streams folder, this one is on the entire project. I realized my mistake too late. I do need some input for this one, I left 2 TODO statements in the code that I'll point out. This commit largely replaces string concatenation with placeholders in debug and trace log statements. Other things I'll point out with comments. For historical purposes, on why replacing string concatenation with placeholders: When running an application on a certain log level, one wants to avoid log statements of more detailed levels actually being evaluated, both due to potential size issues and time spent doing it, since they're not actually going to be printed. If the message being passed to the log method utilizes string concatenation directly in the passed argument, that means the string will be fully evaluated before being passed along to the log method, to then potentially not even be shown, if the application is running on a different log level. Using placeholders prevents this from happening. You can merge this pull request into a Git repository by running: $ git pull https://github.com/KoenDG/kafka logPlaceholders Alternatively you can review and apply these changes as the patch at: https://github.com/apache/kafka/pull/3891.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3891 commit ae179142dc91ab137eb8f0b3ec606f9a12f36f9f Author: coscale_kdegroot Date: 2017-09-18T12:35:26Z WIP, need input on 2 cases in the code. This commit largely replaces string concatenation with placeholders in debug and trace log statements. ---
[GitHub] kafka pull request #3904: [MINOR] Added equals() method to Stamped
GitHub user KoenDG opened a pull request: https://github.com/apache/kafka/pull/3904 [MINOR] Added equals() method to Stamped Please don't let analyzing this PR take away from actual working time. I did this in my spare time, it's not intended to take away from your working hours. It may well be that this is a non-issue. As the title states: I added equals() method to Stamped.java because a class that implements Comparable should always have this. This was pointed out to me by Intellij's code analysis, which warned that classes implementing Comparable should always implement both compareTo() and equals(). This is because an end-user can at some point add objects of that class to java.util.SortedSet. If the compareTo() and equals() implementations are not consistent, that would violate the contract of java.util.Set, which is defined in terms of equals(). findBugs also complained about PunctuationScheduler, a subclass of Stamped, nothing having equals, so I also implemented that. The equals() in Stamped.java is written to be consistent with how the already-existing compareTo() works in that class. The hashCode() is autogenerated, with only the timestamp field, as that is the only one used by compareTo() and equals(). findBugs complained about PunctuationSchedule.java, a subclass of Stamped.java, also needing these methods if Stamped.java had them. So I had equals() and hashCode() auto-generated for them. ./gradlew test ran fine. You can merge this pull request into a Git repository by running: $ git pull https://github.com/KoenDG/kafka stampedEquals Alternatively you can review and apply these changes as the patch at: https://github.com/apache/kafka/pull/3904.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3904 commit aea3d72aa5f6854a845e76eb4359ca456ae7209f Author: coscale_kdegroot Date: 2017-09-19T15:34:32Z Added equals() method to Stamped, because a class that implements Comparable should always have this. findBugs complained about PunctuationSchedula, a subclass of Stamped, nothing having equals, so also implemented that. Since the compareTo and equals methods don't take the value field into account, neither should hashCode. ---
[GitHub] kafka pull request #3919: [MINOR] Where possible, introduce EnumMap.
GitHub user KoenDG opened a pull request: https://github.com/apache/kafka/pull/3919 [MINOR] Where possible, introduce EnumMap. Very simple PR, introduce EnumMap in 2 places of the code. EnumMap: https://docs.oracle.com/javase/7/docs/api/java/util/EnumMap.html EnumMap is meant to be used when the key of a Map is an Enum. From the documentation: > Enum maps are represented internally as arrays. This representation is extremely compact and efficient. Should use less memory per instance and also be faster in lookup. You can merge this pull request into a Git repository by running: $ git pull https://github.com/KoenDG/kafka enummap Alternatively you can review and apply these changes as the patch at: https://github.com/apache/kafka/pull/3919.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3919 commit 6af0ead332c40a531f0974e5ea7c2f97582d18d4 Author: KoenDG Date: 2017-09-20T07:33:54Z Where possible, introduce EnumMap. ---
