MINOR: code cleanup by mjsax · Pull Request #6053 · apache/kafka

mjsax · 2018-12-20T14:18:36Z

While working on #6044, I did code cleanup on the side. Extracted parts of those cleanups into this PR for easier review. This PR should not change any behavior, but do:

Java8 rewrites
removed unused classed
remove warnings
break long lines that are hard to read into short lines
some code re-formatting
JavaDoc improvements

Only change is a dependency change, that I need for #6044, and will make reviewing PRs for KIP-258 simpler.

mjsax

Call for review @guozhangwang @bbejeck @vvcephei

mjsax · 2018-12-20T14:20:25Z

        try {
            if (getTime.shouldRecord()) {
-                return measureLatency(() -> outerValue(inner.get(Bytes.wrap(serdes.rawKey(key)))), getTime);
+                return measureLatency(() -> outerValue(inner.get(keyBytes(key))), getTime);


Extracted into it's own method, to align with code of other stores.

bbejeck

Just one clarifying question, LGTM otherwise.

bbejeck · 2018-12-20T22:36:54Z

        this.map = new TreeMap<>();
    }

-    public KeyValueStore<K, V> enableLogging() {


Does this mean we won't have logging for in-memory key-value stores?

No. This is old code before the state store refactoring via KIP-182. Both, persistent and in-memory key-value store are wrapped with ChangeLoggingKeyValueBytesStore since then, and this method is not called anywhere anymore, and thus the class is not used any longer. Compare Stores and KeyValueStoreBuilder.

Thanks for the clarification. I noticed after I checked out the PR that the enableLogging call was not used at all. I guess I missed that during KIP-182.

👍 Thanks for the catch.

vvcephei

LGTM for the most part! Thanks for cleaning all this up.

vvcephei · 2019-01-02T22:57:12Z

  joptSimple: "net.sf.jopt-simple:jopt-simple:$versions.jopt",
  junit: "junit:junit:$versions.junit",
-  hamcrest: "org.hamcrest:hamcrest-all:1.3",
+  hamcrest: "org.hamcrest:java-hamcrest:$versions.hamcrest",


According to http://hamcrest.org/JavaHamcrest/distributables,
it seems like this should be org:hamcrest:hamcrest:2.1.

But I haven't followed the history of the project. Can you elaborate on the choice of hamcrest-java and 2.0.0.0?

Ah. Good catch. I was checking for hamcrast-all versions and there was no newer one available and was not aware the there is hamcrast, too.

vvcephei · 2019-01-02T22:59:42Z

        final Set<Pattern> sourcePatterns = new HashSet<>();
-        final Set<SourceNodeFactory> sourceNodesForPredecessor = findSourcesForProcessorPredecessors(processorNodeFactory.predecessors);
+        final Set<SourceNodeFactory> sourceNodesForPredecessor
+            = findSourcesForProcessorPredecessors(processorNodeFactory.predecessors);


Nit: seems a little weird to put the assignment operator at the start of this line instead of the end of the previous one.

I agree -- typically only +/- between terms of an expression start a new line

I like it better this way, but I don't care too much either. Will update accordingly.

vvcephei · 2019-01-02T23:00:06Z

                        if (!stateChangelogTopics.containsKey(topicName)) {
-                            final InternalTopicConfig internalTopicConfig = createChangelogTopicConfig(stateFactory, topicName);
+                            final InternalTopicConfig internalTopicConfig
+                                = createChangelogTopicConfig(stateFactory, topicName);


same comment

vvcephei · 2019-01-02T23:00:13Z

    private void adjust(final StreamsConfig config) {
-        final boolean enableOptimization20 = config.getString(StreamsConfig.TOPOLOGY_OPTIMIZATION).equals(StreamsConfig.OPTIMIZE);
+        final boolean enableOptimization20
+            = config.getString(StreamsConfig.TOPOLOGY_OPTIMIZATION).equals(StreamsConfig.OPTIMIZE);


same comment

vvcephei · 2019-01-02T23:00:21Z

            for (final Map.Entry<String, Pattern> stringPatternEntry : nodeToSourcePatterns.entrySet()) {
-                final SourceNodeFactory sourceNode = (SourceNodeFactory) nodeFactories.get(stringPatternEntry.getKey());
+                final SourceNodeFactory sourceNode
+                    = (SourceNodeFactory) nodeFactories.get(stringPatternEntry.getKey());


same comment

vvcephei · 2019-01-02T23:01:27Z

+            final TopologyDescription.Subtopology[] sortedSubtopologies
+                = subtopologies.descendingSet().toArray(new Subtopology[0]);
+            final TopologyDescription.GlobalStore[] sortedGlobalStores
+                = globalStores.descendingSet().toArray(new GlobalStore[0]);


same comment x2

vvcephei · 2019-01-02T23:03:54Z


    /**
-     * Put a key-value pair with the given timestamp into the corresponding window
+     * Put a key-value pair into the window with given window start timestamp


Much better. Thanks!

vvcephei · 2019-01-02T23:05:03Z

        this.map = new TreeMap<>();
    }

-    public KeyValueStore<K, V> enableLogging() {


👍 Thanks for the catch.

vvcephei · 2019-01-02T23:07:51Z

        final List<T> globalStore = globalStoreProvider.stores(storeName, queryableStoreType);
        if (!globalStore.isEmpty()) {
-            return queryableStoreType.create(new WrappingStoreProvider(Collections.<StateStoreProvider>singletonList(globalStoreProvider)), storeName);
+            return queryableStoreType.create(new WrappingStoreProvider(Collections.singletonList(globalStoreProvider)), storeName);


this line is still a bit long... You could try a static import for singletonList.

guozhangwang · 2019-01-03T17:05:29Z

lgtm. @mjsax please feel free to merge after addressing the above comments.

mjsax · 2019-01-03T21:57:25Z

Thanks for the review. Updates this PR. Will merge after build finished.

vvcephei · 2019-01-04T22:29:23Z

👍 Thanks @mjsax !

mjsax · 2019-01-08T21:18:56Z

Retest this please.

mjsax · 2019-01-09T14:02:18Z

Unrelated tests in core and connect failed. Retest this please.

Reviewers: Bill Bejeck <bill@confluent.io>, John Roesler <john@confluent.io>, Ryanne Dolan <ryannedolan@gmail.com>, Guozhang Wang <guozhang@confluent.io>

MINOR: code cleanup

0c7f42b

mjsax added the streams label Dec 20, 2018

mjsax commented Dec 20, 2018

View reviewed changes

This was referenced Dec 20, 2018

MINOR: code cleanup #6054

Merged

MINOR: code cleanup #6056

Merged

bbejeck approved these changes Dec 20, 2018

View reviewed changes

vvcephei reviewed Jan 2, 2019

View reviewed changes

Github comments

42219c8

vvcephei approved these changes Jan 4, 2019

View reviewed changes

mjsax merged commit 1c7bf4e into apache:trunk Jan 9, 2019

mjsax deleted the minor-code-cleanup-main branch January 9, 2019 17:03

pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019

MINOR: code cleanup (apache#6053)

103f400

Reviewers: Bill Bejeck <bill@confluent.io>, John Roesler <john@confluent.io>, Ryanne Dolan <ryannedolan@gmail.com>, Guozhang Wang <guozhang@confluent.io>

Conversation

mjsax commented Dec 20, 2018

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Jan 3, 2019

Uh oh!

mjsax commented Jan 3, 2019

Uh oh!

vvcephei commented Jan 4, 2019

Uh oh!

mjsax commented Jan 8, 2019

Uh oh!

mjsax commented Jan 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mjsax Dec 21, 2018 •

edited

Loading