KAFKA-2358 Cluster collection returning methods never return null #96

sslavic · 2015-07-24T09:05:07Z

See https://issues.apache.org/jira/browse/KAFKA-2358

asfbot · 2015-07-24T09:28:55Z

kafka-trunk-git-pr #37 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-07-24T09:48:52Z

kafka-trunk-git-pr #38 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-07-24T10:17:15Z

kafka-trunk-git-pr #39 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-07-24T10:41:02Z

kafka-trunk-git-pr #40 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-07-24T11:40:18Z

kafka-trunk-git-pr #41 SUCCESS
This pull request looks good

asfbot · 2015-07-24T11:59:28Z

kafka-trunk-git-pr #42 SUCCESS
This pull request looks good

eribeiro · 2015-07-24T16:21:35Z

clients/src/main/java/org/apache/kafka/common/Cluster.java

@@ -151,7 +151,10 @@ public PartitionInfo partition(TopicPartition topicPartition) {
     * @return A list of partitions
     */
    public List<PartitionInfo> partitionsForTopic(String topic) {
-        return this.partitionsByTopic.get(topic);
+        List<PartitionInfo> parts = this.partitionsByTopic.get(topic);
+        if (parts == null)


nit: you could just as well use a ternary operator here:

return (parts == null) ? Collections.<List<PartitionInfo>>emptyList() : parts;

Now I see why it wasn't compiling, the outer List is not needed, just PartitionInfo type is enough.
Fixed, thanks for the tip.

asfbot · 2015-07-24T16:38:36Z

kafka-trunk-git-pr #45 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-07-24T16:43:07Z

kafka-trunk-git-pr #46 FAILURE
Looks like there's a problem with this pull request

sslavic · 2015-07-24T17:04:28Z

@ijuma and @eribeiro thanks for reviewing.
@eribeiro your suggestion doesn't seem to compile. Tried with both explicitly setting type for emptyList() and by having compiler infer it - doesn't work with ternary operator.

asfbot · 2015-07-24T17:09:36Z

kafka-trunk-git-pr #47 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-07-24T17:10:21Z

kafka-trunk-git-pr #48 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-07-24T17:32:36Z

kafka-trunk-git-pr #49 SUCCESS
This pull request looks good

sslavic · 2015-12-17T06:36:20Z

It's been a while, PR which was passing the build before no longer merges with master without conflict, maybe it's no longer relevant even; needs to be checked and rebased if still relevant.

guozhangwang · 2016-01-29T04:12:42Z

@hachikuji Seems still relevant, could you take a look?

hachikuji · 2016-01-29T17:43:45Z

The change LGTM. I think there may be some new usages which need to be updated when rebasing though (e.g. DefaultPartitionGrouper in Kafka Streams still depends on the null value).

…reams-tech-preview HOTFIX: Avoid NPE in StreamsPartitionAssignor

guozhangwang · 2016-12-21T17:55:10Z

@sslavic Could you rebase and address @hachikuji 's comments?

asfbot · 2016-12-21T17:56:21Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/318/
Test FAILed (JDK 8 and Scala 2.12).

sslavic · 2017-01-07T00:10:05Z

@guozhangwang done

asfbot · 2017-01-07T01:09:53Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/593/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-01-07T01:20:55Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/594/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-01-07T02:30:53Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/592/
Test FAILed (JDK 7 and Scala 2.10).

guozhangwang · 2017-01-07T03:27:35Z

test this please

asfbot · 2017-01-07T05:00:43Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/599/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-01-07T05:25:28Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/598/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-01-07T05:25:56Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/600/
Test PASSed (JDK 8 and Scala 2.11).

guozhangwang

Overall LGTM. I have a meta question though: what is the benefit of pushing the null check deeper in the Cluster class than doing it in the caller?

sslavic · 2017-02-07T14:10:10Z

@guozhangwang please see in Effective Java, 2nd edition, "Item 43: Return empty arrays or collections, not nulls"

asfbot · 2017-02-07T14:57:11Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1537/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-02-07T15:03:15Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1534/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-02-07T16:19:48Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1534/
Test FAILed (JDK 8 and Scala 2.12).

guozhangwang · 2017-03-02T21:12:10Z

@sslavic Sorry for the late reply! And thanks for the pointers. LGTM.

guozhangwang · 2017-03-02T21:14:35Z

Merged to trunk (for release version 0.11.0.0).

ijuma · 2017-03-02T21:26:06Z

Btw, strictly speaking this is an incompatible change to a public API class.

guozhangwang · 2017-03-02T21:32:02Z

Thanks for pointing this out @ijuma , and we have been discussing whether these functions should really be considered public APIs (cc @hachikuji @ewencp ).

Currently we have javadocs generated for

  javadoc {
    include "**/org/apache/kafka/clients/consumer/*"
    include "**/org/apache/kafka/clients/producer/*"
    include "**/org/apache/kafka/common/*"
    include "**/org/apache/kafka/common/errors/*"
    include "**/org/apache/kafka/common/serialization/*"
    include "**/org/apache/kafka/common/config/*"
  }

But question is, should all classes within these packages be considered public.

I think ideally we should have an internals package under o.a.k.clients.producer/consumer which contains classes that we do not want to expose as public, while the parent package only expose ProducerRecord, ConsumerRecord, Producer, Consumer, KafkaProducer, KafkaConsumer etc; and o.a.k.common should not be considered public expcet its errors / seralization / config sub-packages. WDYT?

asfbot · 2017-03-02T21:56:53Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1961/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-03-02T21:57:47Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1958/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-03-02T22:27:33Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1959/
Test FAILed (JDK 8 and Scala 2.12).

ijuma · 2017-05-31T06:25:37Z

@guozhangwang sorry for the delay in responding. I agree about having internals packages (we have a few, but this is a bit inconsistent). However, the classes in common have been and should continue to be public. It's too confusing otherwise. If we think it's unlikely that people are relying on the previous behaviour and/or this is like a bug fix, then we should add a note to upgrade.html.

guozhangwang · 2017-05-31T21:55:15Z

It's too confusing otherwise. If we think it's unlikely that people are relying on the previous behaviour and/or this is like a bug fix, then we should add a note to upgrade.html.

Makes sense.

Co-authored-by: Ke Hu <kehu@kehu-mn2.linkedin.biz>

…pache#278) Co-authored-by: Ke Hu <kehu@kehu-mn2.linkedin.biz> (cherry picked from commit c7b3619) Resolved Merge conflicts and corrected tests The upstream behavior has changed in @KAFKA-8904 and @KAFKA-12257 where now merge is called when cluster ids are different in case of a partial update hence changed the called to validateCluster only when it is not a partial update Co-authored-by: Ke Hu <kehu@linkedin.com>

…pache#278) (cherry picked from commit c7b3619) Resolved Merge conflicts and corrected tests The upstream behavior has changed in @KAFKA-8904 and @KAFKA-12257 where now merge is called when cluster ids are different in case of a partial update hence changed the called to validateCluster only when it is not a partial update Co-authored-by: Ke Hu <kehu@linkedin.com>

) Minor revision for KAFKA-14247. Added how the handler is called and constructed to the prototype code path. Reviewers: John Roesler <vvcephei@apache.org>, Kirk True <kirk@mustardgrain.com> Co-authored-by: Philip Nee <pnee@confluent.io>

sslavic force-pushed the feature/KAFKA-2358 branch from a05ed62 to 9cb31cd Compare July 24, 2015 10:07

sslavic changed the title ~~KAFKA-2358 KafkaConsumer.partitionsFor returns empty list for non-existing topic~~ KAFKA-2358 Cluster collection returning methods never return null Jul 24, 2015

sslavic force-pushed the feature/KAFKA-2358 branch 2 times, most recently from 34ad65c to be35ebb Compare July 24, 2015 11:37

eribeiro reviewed Jul 24, 2015
View reviewed changes

sslavic force-pushed the feature/KAFKA-2358 branch from be35ebb to 4293cd9 Compare July 24, 2015 16:37

sslavic force-pushed the feature/KAFKA-2358 branch from 4293cd9 to 938e4d3 Compare July 24, 2015 16:42

sslavic force-pushed the feature/KAFKA-2358 branch 2 times, most recently from 542e8ea to 182d753 Compare July 24, 2015 17:09

sslavic force-pushed the feature/KAFKA-2358 branch from 182d753 to c3df4a5 Compare July 24, 2015 17:14

guozhangwang added a commit to guozhangwang/kafka that referenced this pull request Mar 9, 2016

Merge pull request apache#96 from guozhangwang/0.9.0-cp-2.0.1-with-st…

aabffc9

…reams-tech-preview HOTFIX: Avoid NPE in StreamsPartitionAssignor

KAFKA-2358 Cluster collection returning methods never return null

ccf5537

sslavic force-pushed the feature/KAFKA-2358 branch from ed5dfbd to ccf5537 Compare January 7, 2017 00:29

guozhangwang reviewed Jan 7, 2017

View reviewed changes

asfgit closed this in ca06862 Mar 2, 2017

xiowu0 pushed a commit to xiowu0/kafka that referenced this pull request Dec 3, 2020

[LI-HOTFIX] Reject stale metadata from different cluster (apache#96)

c7b3619

Co-authored-by: Ke Hu <kehu@kehu-mn2.linkedin.biz>

KAFKA-2358 Cluster collection returning methods never return null #96

KAFKA-2358 Cluster collection returning methods never return null #96

Conversation

sslavic commented Jul 24, 2015

asfbot commented Jul 24, 2015

asfbot commented Jul 24, 2015

asfbot commented Jul 24, 2015

asfbot commented Jul 24, 2015

asfbot commented Jul 24, 2015

asfbot commented Jul 24, 2015

eribeiro Jul 24, 2015

Choose a reason for hiding this comment

sslavic Jan 5, 2017 • edited

Choose a reason for hiding this comment

asfbot commented Jul 24, 2015

asfbot commented Jul 24, 2015

sslavic commented Jul 24, 2015

asfbot commented Jul 24, 2015

asfbot commented Jul 24, 2015

asfbot commented Jul 24, 2015

sslavic commented Dec 17, 2015

guozhangwang commented Jan 29, 2016

hachikuji commented Jan 29, 2016

guozhangwang commented Dec 21, 2016

asfbot commented Dec 21, 2016

sslavic commented Jan 7, 2017

asfbot commented Jan 7, 2017

asfbot commented Jan 7, 2017

asfbot commented Jan 7, 2017

guozhangwang commented Jan 7, 2017

asfbot commented Jan 7, 2017

asfbot commented Jan 7, 2017

asfbot commented Jan 7, 2017

guozhangwang left a comment

Choose a reason for hiding this comment

sslavic commented Feb 7, 2017

asfbot commented Feb 7, 2017

asfbot commented Feb 7, 2017

asfbot commented Feb 7, 2017

guozhangwang commented Mar 2, 2017

guozhangwang commented Mar 2, 2017

ijuma commented Mar 2, 2017

guozhangwang commented Mar 2, 2017

asfbot commented Mar 2, 2017

asfbot commented Mar 2, 2017

asfbot commented Mar 2, 2017

ijuma commented May 31, 2017

guozhangwang commented May 31, 2017

sslavic Jan 5, 2017 •

edited