KAFKA-7612: Fix javac warnings and enable warnings as errors by ijuma · Pull Request #5900 · apache/kafka

ijuma · 2018-11-11T10:40:29Z

Use Xlint:all with 3 exclusions (filed KAFKA-7613 to remove the exclusions)
Use the same javac options when compiling tests (seems accidental that
we didn't do this before)
Replaced several deprecated method calls with non-deprecated ones:
- KafkaConsumer.poll(long) and KafkaConsumer.close(long)
- Class.newInstance and new Integer/Long (deprecated since Java 9)
- scala.Console (deprecated in Scala 2.11)
- PartitionData taking a timestamp (one of them seemingly a bug)
- JsonMappingException single parameter constructor
Fix unnecessary usage of raw types in several places.
Add @SuppressWarnings for deprecations, unchecked and switch fallthrough in
several places.
Scala clean-ups (var -> val, ETA expansion warnings, avoid reflective calls)
Use lambdas to simplify code in a few places
Add @SafeVarargs, fix varargs usage and remove unnecessary Utils.mkList method

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

ijuma · 2018-11-11T10:43:34Z

@rhauch Can you please review the Connect changes that start from this point?

ijuma · 2018-11-11T10:43:58Z

@mjsax @guozhangwang Can you please review the Streams changes that start from this point.

mjsax · 2018-11-11T18:51:24Z

\cc @bbejeck Can you have a look here? You know the optimizer code best.

@ijuma I looked into this in more detail. We don't need a new type R -- instead, we should update to

public class KTableKTableJoinNode<K, V1, V2, VR> extends BaseJoinProcessorNode<K, Change<V1>, Change<V2>, Change<VR>>

and use private final MaterializedInternal<K, VR, KeyValueStore<Bytes, byte[]>> materializedInternal; (ie, VR instead of R. (Note, the you always pass <..., Change<X>, X> in the current PR, what is redundant and can be avoided.)

@mjsax That doesn't work, it's what I tried first. As far as I can tell, the existing code was wrong but it doesn't matter due to the way generics are used here.

Synced offline, I had misunderstood the suggestion.

@mjsax Good suggestion, I've added a commit. Let me know what you think.

the changes to the optimizer code LGTM

mjsax

As far as I can tell, LGTM.

Not sure about the Scala change and why we need to additional generic type though.

…eprecated overload

ijuma · 2018-11-12T08:53:20Z

@omkreddy Do you have time to review this?

omkreddy

core and clients module changes are looks good to me.

ijuma · 2018-11-12T18:41:52Z

JDK 11 build passed and JDK 8 failures are unrelated. @mjsax, can you please take a look at the Connect changes? They're pretty trivial and that would give us enough committer coverage to get this merged before it goes stale. :)

ijuma · 2018-11-12T18:44:39Z

  private val uncleanablePartitions = mutable.HashMap[String, mutable.Set[TopicPartition]]()

-  /* the set of directories marked as uncleanable and therefore offline */
-  private val uncleanableDirs = mutable.HashSet[String]()


cc @stanislavkozlovski

looks good. thanks

ijuma · 2018-11-12T18:44:48Z

  // Visible for testing
  private[server] def getFetcher(topicPartition: TopicPartition): Option[T] = {
    lock synchronized {
-      val fetcherId = getFetcherId(topicPartition)


cc @hachikuji

rhauch

Connect changes look good to me, tho I'm not a committer.

…rameters

ijuma · 2018-11-13T06:16:47Z

Only testCoordinatorFailover failed and it's unrelated (there's a separate PR to fix that). I'll go ahead and merge this. If there are any additional comments, I'll follow-up in a subsequent PR.

guozhangwang

Made a pass over streams part. LGTM except one question (above).

I've also made a minor PR for replacing some deprecated functions, maybe better merging this one and rebase:

#5911

guozhangwang · 2018-11-14T02:29:45Z

                return viewByRegion;
            })
            .map((user, viewRegion) -> new KeyValue<>(viewRegion.region, viewRegion))
-            .groupByKey(Serialized.with(Serdes.String(), new JSONSerde<>()))


We can replace Serialized with Grouped, ditto below.

cc @bbejeck

guozhangwang · 2018-11-14T02:30:18Z

    @Deprecated
    public <KR> KGroupedStream<KR, V> groupBy(final KeyValueMapper<? super K, ? super V, KR> selector,
-                                              final Serialized<KR, V> serialized) {
+                                              final org.apache.kafka.streams.kstream.Serialized<KR, V> serialized) {


guozhangwang · 2018-11-14T02:34:29Z

 * Too much specific information to generalize so the KTable-KTable join requires a specific node.
 */
-public class KTableKTableJoinNode<K, V1, V2, VR> extends BaseJoinProcessorNode<K, V1, V2, VR> {
+public class KTableKTableJoinNode<K, V1, V2, VR> extends BaseJoinProcessorNode<K, Change<V1>, Change<V2>, Change<VR>> {


What's the purpose of moving Change<V> inside this class instead of on the callers?

That was @mjsax suggestion to avoid adding an additional type parameter for Materialized. See the last commit in the PR.

guozhangwang · 2018-11-14T02:39:14Z

            .groupBy(
                mapper,
-                Serialized.with(Serdes.String(), Serdes.String()));
+                org.apache.kafka.streams.kstream.Serialized.with(Serdes.String(), Serdes.String()));


Ditto here and below: use Grouped instead of Serialized.

guozhangwang · 2018-11-14T02:39:53Z

            .groupBy(
                mapper,
-                Serialized.with(Serdes.String(), Serdes.String()));
+                org.apache.kafka.streams.kstream.Serialized.with(Serdes.String(), Serdes.String()));


Ditto here and below.

guozhangwang · 2018-11-14T02:43:11Z

 import static org.junit.Assert.fail;

-
+@SuppressWarnings("deprecation")


We should replace until with grace here, cc @vvcephei

guozhangwang · 2018-11-14T02:44:13Z

 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;

+@SuppressWarnings("deprecation")


We should replace of with the other overloaded function. cc @vvcephei

ijuma · 2018-11-14T03:46:33Z

@guozhangwang Thanks for the review. I responded to the question about the type parameters. All the comments about removing deprecated usage with the undeprecated alternative seem to be addressed in your follow-up PR so we can just discuss it there. This one was already merged

guozhangwang · 2018-11-14T18:05:28Z

Yeah I guess it was merged while I was preparing the PR, and I did not notice it after rebasing trunk :) Sounds good.

@SuppressWarnings

…5900) - Use Xlint:all with 3 exclusions (filed KAFKA-7613 to remove the exclusions) - Use the same javac options when compiling tests (seems accidental that we didn't do this before) - Replaced several deprecated method calls with non-deprecated ones: - `KafkaConsumer.poll(long)` and `KafkaConsumer.close(long)` - `Class.newInstance` and `new Integer/Long` (deprecated since Java 9) - `scala.Console` (deprecated in Scala 2.11) - `PartitionData` taking a timestamp (one of them seemingly a bug) - `JsonMappingException` single parameter constructor - Fix unnecessary usage of raw types in several places. - Add @SuppressWarnings for deprecations, unchecked and switch fallthrough in several places. - Scala clean-ups (var -> val, ETA expansion warnings, avoid reflective calls) - Use lambdas to simplify code in a few places - Add @SafeVarargs, fix varargs usage and remove unnecessary `Utils.mkList` method Reviewers: Matthias J. Sax <mjsax@apache.org>, Manikumar Reddy <manikumar.reddy@gmail.com>, Randall Hauch <rhauch@gmail.com>, Bill Bejeck <bill@confluent.io>, Stanislav Kozlovski <stanislav_kozlovski@outlook.com>

Use emptySet() instead of EMPTY_SET

69738ef

ijuma changed the title ~~KAFKA-7612: Fix javac warnings and enable warnings as errors~~ KAFKA-7612: Fix javac warnings and enable warnings as errors [WIP] Nov 11, 2018

ijuma commented Nov 11, 2018

View reviewed changes

mjsax reviewed Nov 11, 2018

View reviewed changes

ijuma force-pushed the kafka-7612-fix-javac-warnings branch 2 times, most recently from a8e6887 to be7b014 Compare November 12, 2018 02:29

ijuma added 22 commits November 12, 2018 00:30

Use KafkaConsumer.poll(Duration) and close(Duration) instead of d…

44346a9

…eprecated overload

Suppress switch fallthrough warnings

892082c

var -> val

7050f42

Console -> StdIn

90144cc

ETA expansion warning fixes

9771049

Class.newInstance was deprecated in Java 9

3bdb9fc

Avoid reflective call warnings

9cb13b6

Use lambda

280982b

Use constructor that takes a leader epoch

bd3fd96

Fix most deprecation warnings

6e62ac5

Remove unused variables/code/imports

683a9b7

Remove Utils.mkList and fix mkSet warnings

ea288ee

new Long/Integer -> valueOf

5a5ff2b

Connect unchecked, varargs and lambda fixes/improvements

58160a6

Fix missing type parameters in Streams

56e9b7f

Add unchecked warning suppressions for Streams

11a5f44

Avoid JsonMappingException deprecation warnings

923884b

Fix clients raw type usage

16b98e5

Suppress unchecked warnings in clients

f8fa392

Connect unchecked suppressions

78161dc

Treat javac warnings as errors and tweak Xlint to include more warnings

aae47c0

Fix javac warnings for clients tests

116dc1c

ijuma added 10 commits November 12, 2018 00:30

Fix javac warnings for Streams tests

5a34774

Fix javac warnings for Connect tests

644ec1a

Fix javac warning for trogdor test

a34a061

Suppress all unchecked warnings for Streams tests with 8+ such warnings

d4150d6

javac options should also apply to tests

f209deb

Streams tests javac warnings test

5edd693

Suppress Streams test deprecation warnings

7a2dd14

Suppress connect test deprecation warnings

16c0cca

Enable deprecation warnings

5349736

Remove unnecessary code in EpochDrivenReplicationProtocolAcceptableTest

797ec97

ijuma force-pushed the kafka-7612-fix-javac-warnings branch from 870081c to 797ec97 Compare November 12, 2018 08:30

ijuma changed the title ~~KAFKA-7612: Fix javac warnings and enable warnings as errors [WIP]~~ KAFKA-7612: Fix javac warnings and enable warnings as errors Nov 12, 2018

omkreddy approved these changes Nov 12, 2018

View reviewed changes

ijuma commented Nov 12, 2018

View reviewed changes

rhauch approved these changes Nov 12, 2018

View reviewed changes

Address Matthias' suggestion to simplify KTableKTableJoinNode type pa…

7d2c25b

…rameters

ijuma force-pushed the kafka-7612-fix-javac-warnings branch from 7aa0320 to 7d2c25b Compare November 12, 2018 21:38

ijuma merged commit 12f310d into apache:trunk Nov 13, 2018

ijuma deleted the kafka-7612-fix-javac-warnings branch November 13, 2018 06:19

guozhangwang reviewed Nov 14, 2018

View reviewed changes

		import static org.junit.Assert.fail;


		@SuppressWarnings("deprecation")

Conversation

ijuma commented Nov 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

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!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Nov 12, 2018

Uh oh!

omkreddy left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Nov 12, 2018

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!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Nov 13, 2018

Uh oh!

guozhangwang 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!

ijuma commented Nov 14, 2018

Uh oh!

guozhangwang commented Nov 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ijuma commented Nov 11, 2018 •

edited

Loading