KAFKA-8240: Fix NPE in Source.equals() by mjsax · Pull Request #6589 · apache/kafka

mjsax · 2019-04-17T01:15:44Z

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

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)

mjsax · 2019-04-17T01:16:15Z

Call for review @guozhangwang @bbejeck @vvcephei @ableegoldman @cadonna

mjsax · 2019-04-17T07:22:38Z

While debugging the issue, I figure that the use comparators don't do deep comparison. This is the corresponding fix. (Similar below)

mjsax · 2019-04-17T07:23:54Z

Just a basic test for null pattern.

nit: Could you rename this test so that it reflects the focus on the null pattern?

Ack. Renamed this one and added a new test for Pattern case.

Note, we know that either one of both is null.

vvcephei

Thanks!

mjsax · 2019-04-17T16:08:50Z

Java8 failed with know flaky test. Java11 passed.

Retest this please.

bbejeck · 2019-04-18T16:26:53Z

Java 8 passed, Java 11 failed; test results already cleaned up

retest this please

bbejeck · 2019-04-18T18:05:44Z

Failed with Execution failed for task ':core:compileTestScala'. but a PR (#6603) has been merged for this.

retest this please

mjsax · 2019-04-19T02:08:44Z

Rebased.

mjsax · 2019-04-20T02:01:55Z

Updated this.

mjsax · 2019-04-22T16:28:36Z

@bbejeck @vvcephei @cadonna Build finally passed. I change couple of things. Would like to get another review to make sure it's still ok.

cadonna · 2019-04-22T16:54:28Z

+
+    @Test(expected = NullPointerException.class)
+    public void shouldThrowIfNameIsNull() {
+        new InternalTopologyBuilder.Source(null, Collections.emptySet(), null);


Here and for the next two tests, it would be better to rewrite the tests to following:

final Exception e = assertThrows(NullPointerException.class, () -> new InternalTopologyBuilder.Source(null, Collections.emptySet(), null); assertEquals(<expected message>, e.getMessage());

We get an NPE with null message here. Not sure if this improves the test? Let me know you think. We cannot set a message because we use Objects.requireNonNull that trows the NPE.

OK, I overlooked that the overload of requireNonNull without the message is used in the code under test. However, I think it would be better to use the overload with message, i.e.,

public static <T> T requireNonNull(T obj, String message)

Well. We never specify a message for internal checks. It's not a user facing API but effectively an internal assertion. So not sure if we gain much adding a message and checking for it. It's a fatal error anyway if we hit the NPE.

If we think we should specify messages, we should change our pattern and do this consistently. Thoughts?

OK, makes sense. Didn't know about policy of internal checks. Would be good to have it written down somewhere.

Do you mean the assert keyword in Java? IIUC assertions need to explicitly turned on at execution time. So you need to rely on the user to turn them on.

Yes, I mean assert keyword. If we turn it on in our tests, it might be sufficient (it's internal checks)? Not sure.

Other suggestions are more than welcome :)

My preference would be requireNonNull and add the message. First IMHO it's always better to have some sort of description v.s just a NullPointerException and second, there is less chance for error since we need to explicitly enable assertions. Just my 2 cents.

I am with @bbejeck on this. Alternatively, we could specify a separate exception that shows that a contract was violated internally and gives you information to quickly identify the bug. However, I am not sure whether this additional exception would introduce more complexity than benefits.

I would not use an assert and turn it on only in tests. If a contract is violated internally during production, it could prolong the search for the bug, because the error could pop up in places other than the actual origin of the violation.

cadonna · 2019-04-22T17:18:56Z

+    public void shouldCompareSourceNodeWithTopicPattern() {
+        final InternalTopologyBuilder.Source base = new InternalTopologyBuilder.Source("name", null, Pattern.compile("topic"));
+        final InternalTopologyBuilder.Source sameAsBase = new InternalTopologyBuilder.Source("name", null, Pattern.compile("topic"));
+        final InternalTopologyBuilder.Source differentName = new InternalTopologyBuilder.Source("name2", null, Pattern.compile("topic"));


You basically specified the differentName check twice, in this and in the previous method. I would specify the following tests into separate methods:

same

different names

different topics

different patterns
Makes the test better readable and avoids repetitions of checks.

cadonna · 2019-04-22T17:20:22Z

+            if (topics != null && pattern != null) {
+                throw new IllegalArgumentException("Either topics or pattern must be null, but both are not null.");
+            }
+


What should happen if the topic or the pattern is empty?

I don't think those are special cases. For Pattern.compile(null) it throws NPE, so we would never hit this code but fail before.

Pattern.compile("") should be fine but just don't match any topic. Similar, to empty topic list. For both cases, the consumer should just be idle.

\cc @guozhangwang or @hachikuji to confirm.

mjsax · 2019-04-23T16:53:07Z

Updated this.

bbejeck · 2019-04-25T05:25:57Z

+
+    @Test(expected = NullPointerException.class)
+    public void shouldThrowIfNameIsNull() {
+        new InternalTopologyBuilder.Source(null, Collections.emptySet(), null);


My preference would be requireNonNull and add the message. First IMHO it's always better to have some sort of description v.s just a NullPointerException and second, there is less chance for error since we need to explicitly enable assertions. Just my 2 cents.

daiweisong · 2019-04-25T07:30:46Z

niubi

mjsax · 2019-04-27T21:35:32Z

Updated this.

cadonna · 2019-05-03T08:36:24Z

LGTM

bbejeck · 2019-05-03T14:33:05Z

Merged #6589 to trunk

- backport of PR apache#6589 to 2.2 branch

- backport of PR #6589 to 2.2 branch Reviewers: Bruno Cadonna <bruno@confluent.io>, Bill Bejeck <bill@confluent.io>

Reviewers: John Roesler <john@confluent.io>, Bruno Cadonna <bruno@confluent.io>, Bill Bejeck <bbejeck@gmail.com>

mjsax added the streams label Apr 17, 2019

mjsax commented Apr 17, 2019

View reviewed changes

vvcephei approved these changes Apr 17, 2019

View reviewed changes

bbejeck approved these changes Apr 17, 2019

View reviewed changes

cadonna approved these changes Apr 18, 2019

View reviewed changes

mjsax added 3 commits April 18, 2019 19:08

KAFKA-8240: Fix NPE in Source.equals()

67f8f8b

Fix

ebde0b9

Restore comment

1e44568

mjsax force-pushed the kafka-8240-npe-topology-describe branch from 113950d to 1e44568 Compare April 19, 2019 02:08

Github comments

33b9e3a

mjsax added 2 commits April 20, 2019 13:42

Github comments

ea3e498

Fix failing test

ff0e74e

cadonna reviewed Apr 22, 2019

View reviewed changes

Github comments

5f2a96e

bbejeck approved these changes Apr 25, 2019

View reviewed changes

Github comments

ff1c570

bbejeck merged commit 6d3ff13 into apache:trunk May 3, 2019

mjsax deleted the kafka-8240-npe-topology-describe branch May 6, 2019 13:06

mjsax added a commit to mjsax/kafka that referenced this pull request May 6, 2019

KAFKA-8240: Fix NPE in Source.equals()

44e1d70

- backport of PR apache#6589 to 2.2 branch

mjsax mentioned this pull request May 6, 2019

KAFKA-8240: Fix NPE in Source.equals() #6685

Merged

mjsax added a commit that referenced this pull request May 9, 2019

KAFKA-8240: Fix NPE in Source.equals() (#6685)

17708df

- backport of PR #6589 to 2.2 branch Reviewers: Bruno Cadonna <bruno@confluent.io>, Bill Bejeck <bill@confluent.io>

mjsax added a commit that referenced this pull request May 9, 2019

KAFKA-8240: Fix NPE in Source.equals() (#6685)

3384a9d

- backport of PR #6589 to 2.2 branch Reviewers: Bruno Cadonna <bruno@confluent.io>, Bill Bejeck <bill@confluent.io>

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

KAFKA-8240: Fix NPE in Source.equals() (apache#6589)

a646f49

Reviewers: John Roesler <john@confluent.io>, Bruno Cadonna <bruno@confluent.io>, Bill Bejeck <bbejeck@gmail.com>

Conversation

mjsax commented Apr 17, 2019

Committer Checklist (excluded from commit message)

Uh oh!

mjsax commented Apr 17, 2019

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!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax commented Apr 17, 2019

Uh oh!

bbejeck commented Apr 18, 2019

Uh oh!

bbejeck commented Apr 18, 2019

Uh oh!

mjsax commented Apr 19, 2019

Uh oh!

mjsax commented Apr 20, 2019

Uh oh!

mjsax commented Apr 22, 2019

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!

Choose a reason for hiding this comment

Uh oh!

mjsax commented Apr 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daiweisong commented Apr 25, 2019

Uh oh!

mjsax commented Apr 27, 2019

Uh oh!

cadonna commented May 3, 2019

Uh oh!

bbejeck commented May 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants