You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/06/09 16:06:57 UTC
[GitHub] [kafka] jlprat opened a new pull request #10856: MINOR: Small optimizations and removal of unused code in Streams
jlprat opened a new pull request #10856:
URL: https://github.com/apache/kafka/pull/10856
Remove unused methods in internal classes
Mark fields that can be final as final
Remove unneeded generic type annotation
Convert single use fields to local final variables
Use method reference in lambdas when it's more readable
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #10856: MINOR: Small optimizations and removal of unused code in Streams
Posted by GitBox <gi...@apache.org>.
cadonna commented on pull request #10856:
URL: https://github.com/apache/kafka/pull/10856#issuecomment-858494872
I restarted the checks since all three builds failed with exit code 1, which seems to be related to https://issues.apache.org/jira/browse/KAFKA-12892
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on pull request #10856: MINOR: Small optimizations and removal of unused code in Streams
Posted by GitBox <gi...@apache.org>.
jlprat commented on pull request #10856:
URL: https://github.com/apache/kafka/pull/10856#issuecomment-858573500
Yes, I see that the part of the build running the test is finishing with exit code 1. The one checking the compilation, spotbugs, and co finished successfully, though.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a change in pull request #10856: MINOR: Small optimizations and removal of unused code in Streams
Posted by GitBox <gi...@apache.org>.
cadonna commented on a change in pull request #10856:
URL: https://github.com/apache/kafka/pull/10856#discussion_r648988189
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java
##########
@@ -562,10 +553,6 @@ private String externalChildSensorName(final String threadId, final String opera
+ SENSOR_NAME_DELIMITER + operationName;
}
- private String externalParentSensorName(final String threadId, final String operationName) {
Review comment:
Same here
##########
File path: streams/src/main/java/org/apache/kafka/streams/StoreQueryParameters.java
##########
@@ -25,8 +25,8 @@
*/
public class StoreQueryParameters<T> {
- private Integer partition;
- private boolean staleStores;
+ private final Integer partition;
+ private final boolean staleStores;
Review comment:
The checkstyle rule used only checks local variables, not member fields:
```
<!-- variables that can be final should be final (suppressed except for Streams) -->
<module name="FinalLocalVariable">
<property name="tokens" value="VARIABLE_DEF,PARAMETER_DEF"/>
<property name="validateEnhancedForLoopVariable" value="true"/>
</module>
```
See https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheck.html
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java
##########
@@ -294,14 +293,6 @@ public final void removeAllThreadLevelSensors(final String threadId) {
return tagMap;
}
- public Map<String, String> bufferLevelTagMap(final String threadId,
Review comment:
That is fine! Apparently I missed this method when I removed the old Streams metrics structure. Thanks @jlprat !
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on pull request #10856: MINOR: Small optimizations and removal of unused code in Streams
Posted by GitBox <gi...@apache.org>.
jlprat commented on pull request #10856:
URL: https://github.com/apache/kafka/pull/10856#issuecomment-857842621
Pinging @mjsax on this one as you did some changes on Streams recently. Thanks!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on pull request #10856: MINOR: Small optimizations and removal of unused code in Streams
Posted by GitBox <gi...@apache.org>.
jlprat commented on pull request #10856:
URL: https://github.com/apache/kafka/pull/10856#issuecomment-858471996
Thanks both for the review!
Shall I do something else, or is it ready to merge?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on a change in pull request #10856: MINOR: Small optimizations and removal of unused code in Streams
Posted by GitBox <gi...@apache.org>.
jlprat commented on a change in pull request #10856:
URL: https://github.com/apache/kafka/pull/10856#discussion_r648867754
##########
File path: streams/src/main/java/org/apache/kafka/streams/StoreQueryParameters.java
##########
@@ -25,8 +25,8 @@
*/
public class StoreQueryParameters<T> {
- private Integer partition;
- private boolean staleStores;
+ private final Integer partition;
+ private final boolean staleStores;
Review comment:
As far as I can see, these fields are only assigned at constructor, and are never modified. Being this, together with in place initialization, the only valid patterns for final fields.
Or am I missing something?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on pull request #10856: MINOR: Small optimizations and removal of unused code in Streams
Posted by GitBox <gi...@apache.org>.
jlprat commented on pull request #10856:
URL: https://github.com/apache/kafka/pull/10856#issuecomment-858645362
Some steps of the build seem to have passed while some others failed
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #10856: MINOR: Small optimizations and removal of unused code in Streams
Posted by GitBox <gi...@apache.org>.
cadonna commented on pull request #10856:
URL: https://github.com/apache/kafka/pull/10856#issuecomment-858689473
JDK 11 and ARM passed. Failed tests are unrelated and the issue is known.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on a change in pull request #10856: MINOR: Small optimizations and removal of unused code in Streams
Posted by GitBox <gi...@apache.org>.
jlprat commented on a change in pull request #10856:
URL: https://github.com/apache/kafka/pull/10856#discussion_r648463106
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/graph/BaseJoinProcessorNode.java
##########
@@ -63,10 +63,6 @@
return joinMergeProcessorParameters;
}
- ValueJoinerWithKey<? super K, ? super V1, ? super V2, ? extends VR> valueJoiner() {
Review comment:
As it is an internal class and this method is not use in the code base, I assume is safe to delete.
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/foreignkeyjoin/CombinedKey.java
##########
@@ -39,9 +39,6 @@ public KP getPrimaryKey() {
}
public boolean equals(final KF foreignKey, final KP primaryKey) {
- if (this.primaryKey == null) {
Review comment:
This is removed because `primaryKey` is guaranteed in constructor to never be null.
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java
##########
@@ -294,14 +293,6 @@ public final void removeAllThreadLevelSensors(final String threadId) {
return tagMap;
}
- public Map<String, String> bufferLevelTagMap(final String threadId,
Review comment:
This public function on an internal class is never used in our code base, so I assume it is safe to delete.
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/TableTableJoinIntegrationTest.java
##########
@@ -59,11 +59,11 @@ public void prepareTopology() throws InterruptedException {
rightTable = builder.table(INPUT_TOPIC_RIGHT, Materialized.<Long, String, KeyValueStore<Bytes, byte[]>>as("right").withLoggingDisabled());
}
- final private TestRecord<Long, String> expectedFinalJoinResult = new TestRecord<>(ANY_UNIQUE_KEY, "D-d", null, 15L);
- final private TestRecord<Long, String> expectedFinalMultiJoinResult = new TestRecord<>(ANY_UNIQUE_KEY, "D-d-d", null, 15L);
- final private String storeName = appID + "-store";
+ private final TestRecord<Long, String> expectedFinalJoinResult = new TestRecord<>(ANY_UNIQUE_KEY, "D-d", null, 15L);
Review comment:
Just using the `private final` order consistently
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java
##########
@@ -562,10 +553,6 @@ private String externalChildSensorName(final String threadId, final String opera
+ SENSOR_NAME_DELIMITER + operationName;
}
- private String externalParentSensorName(final String threadId, final String operationName) {
Review comment:
This private function was never used
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java
##########
@@ -246,15 +245,15 @@ public final void removeAllClientLevelSensorsAndMetrics() {
removeAllClientLevelMetrics();
}
- private final void removeAllClientLevelMetrics() {
Review comment:
`private` and `final` together doesn't really make sense
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #10856: MINOR: Small optimizations and removal of unused code in Streams
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10856:
URL: https://github.com/apache/kafka/pull/10856#discussion_r648739553
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java
##########
@@ -294,14 +293,6 @@ public final void removeAllThreadLevelSensors(final String threadId) {
return tagMap;
}
- public Map<String, String> bufferLevelTagMap(final String threadId,
Review comment:
\cc @cadonna Any though on this? (Similar below)
##########
File path: streams/src/main/java/org/apache/kafka/streams/StoreQueryParameters.java
##########
@@ -25,8 +25,8 @@
*/
public class StoreQueryParameters<T> {
- private Integer partition;
- private boolean staleStores;
+ private final Integer partition;
+ private final boolean staleStores;
Review comment:
Wondering who this is possible? Should the build not fail for this case? \cc @vvcephei
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jlprat commented on pull request #10856: MINOR: Small optimizations and removal of unused code in Streams
Posted by GitBox <gi...@apache.org>.
jlprat commented on pull request #10856:
URL: https://github.com/apache/kafka/pull/10856#issuecomment-858694138
Thanks both for the reviews
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on a change in pull request #10856: MINOR: Small optimizations and removal of unused code in Streams
Posted by GitBox <gi...@apache.org>.
vvcephei commented on a change in pull request #10856:
URL: https://github.com/apache/kafka/pull/10856#discussion_r649509926
##########
File path: streams/src/main/java/org/apache/kafka/streams/StoreQueryParameters.java
##########
@@ -25,8 +25,8 @@
*/
public class StoreQueryParameters<T> {
- private Integer partition;
- private boolean staleStores;
+ private final Integer partition;
+ private final boolean staleStores;
Review comment:
Yeah, it's a limitation of checkstyle, which makes me a bit sad.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna merged pull request #10856: MINOR: Small optimizations and removal of unused code in Streams
Posted by GitBox <gi...@apache.org>.
cadonna merged pull request #10856:
URL: https://github.com/apache/kafka/pull/10856
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org