KAFKA-12914: StreamSourceNode should return `null` topic name for pattern subscription by mjsax · Pull Request #10846 · apache/kafka

mjsax · 2021-06-08T19:02:06Z

Call for review @cadonna

…tern subscription

showuon

LGTM! Just a thought that do you think it's better to return empty set when null?

mjsax · 2021-06-09T02:48:52Z

Just a thought that do you think it's better to return empty set when null?

For SourceGraphNode we either set topicName or pattern null, and we also return null for the pattern case if topicName is used and pattern is null. So I thought it might be better aligned to just return nulliftopicNameisnull`, too. But I don't have a strong opinion.

As an afterthought, it might even be better to change both return types to Optional ?

showuon · 2021-06-09T02:52:32Z

As an afterthought, it might even be better to change both return types to Optional ?

Sounds good!

cadonna · 2021-06-09T07:57:01Z

Optional sounds good but it would substantially increase the size of this fix which is actually not required.

On the other hand, I am not sure if we do not have other related NPE lurking in the code that should be found by changing to Optional. For example, I could not find a null check for

kafka/streams/src/main/java/org/apache/kafka/streams/kstream/internals/graph/TableSourceNode.java

Line 86 in 45d7440

final String topicName = topicNames().iterator().next();

mjsax · 2021-06-09T23:12:51Z

What do you propose @cadonna ? Also happy to do a follow up PR (but not sure if necessary)? It's all just internal code so we can change at will. Happy to merge as-is, or change to Optional in this or a follow up PR...

cadonna

@mjsax , I am saying that the PR is fine as it is because it fixes the bug described in the ticket. If you want to avoid potentially lurking bugs, you could change it to Optional. But that can be done in a follow-up PR. Your call.

mjsax · 2021-06-11T03:30:54Z

Update the PR to change the return type to Optional. If no objections are raised and Jenkins passed, I plan to merge this next week.

guozhangwang

LGTM.

…tern subscription (#10846) Reviewers: Luke Chen <showuon@gmail.com>, Bruno Cadonna <bruno@confluent.io>, Guozhang Wang <guozhang@confluent.io>

mjsax · 2021-06-15T08:15:25Z

Merged to trunk and cherry-picked to 2.8 branch.

@ableegoldman

Resolve merge conflicts in Jenkins file. * MINOR: clean up unneeded `@SuppressWarnings` (apache#10855) Reviewers: Luke Chen <showuon@gmail.com>, Matthias J. Sax <mjsax@apache.org>, Chia-Ping Tsai <chia7712@gmail.com> * KAFKA-12940: Enable JDK 16 builds in Jenkins (apache#10702) JDK 15 no longer receives updates, so we want to switch from JDK 15 to JDK 16. However, we have a number of tests that don't yet pass with JDK 16. Instead of replacing JDK 15 with JDK 16, we have both for now and we either disable (via annotations) or exclude (via gradle) the tests that don't pass with JDK 16 yet. The annotations approach is better, but it doesn't work for tests that rely on the PowerMock JUnit 4 runner. Also add `--illegal-access=permit` when building with JDK 16 to make MiniKdc work for now. This has been removed in JDK 17, so we'll have to figure out another solution when we migrate to that. Relevant JIRAs for the disabled tests: KAFKA-12790, KAFKA-12941, KAFKA-12942. Moved some assertions from `testTlsDefaults` to `testUnsupportedTlsVersion` since the former claims to test the success case while the former tests the failure case. Reviewers: Chia-Ping Tsai <chia7712@gmail.com> * KAFKA-12921: Upgrade zstd-jni to 1.5.0-2 (apache#10847) This PR aims to upgrade `zstd-jni` from `1.4.9-1` to `1.5.0-2`. This change will incorporate a number of bug fixes and performance improvements made in `1.5.0` of `zstd`: - https://github.com/facebook/zstd/releases/tag/v1.5.0 - https://github.com/luben/zstd-jni/releases/tag/v1.5.0-1 - https://github.com/luben/zstd-jni/releases/tag/v1.5.0-2 The most recent `1.5.0` release offers +25%-140% (compression) and +15% (decompression) performance improvements under certain conditions. Those conditions are unlikely to apply to Kafka with the default configuration, however. Since this is a dependency change, this should pass all the existing CIs. Reviewers: Lee Dongjin <dongjin@apache.org>, Ismael Juma <ismael@juma.me.uk> * KAFKA-8940: decrease session timeout to make test faster and reliable (apache#10871) While there might still be some issue about the test as described here by @ableegoldman , but I found the reason why this test failed quite frequently recently. It's because we increased the session timeout to 45 sec in KIP-735. The reason why increasing session timeout affected this test is because in this test, we will keep adding new stream clients and remove old one, to maintain only 3 stream clients alive. The problem here is, when old stream closed, we won't trigger rebalance immediately due to the stream clients are all static members as described in KIP-345, which means, we will trigger trigger group rebalance only when session.timeout expired. That said, when old client closed, we'll have at least 45 sec with some tasks not working. Also, in this test, we have 2 timeout conditions to fail this test before verification passed: 1. 6 minutes timeout 2. polling 30 times (each with 5 seconds) without getting any data. (that is, 5 * 30 = 150 sec without consuming any data) For (1), in my test under 45 session timeout, we'll create 8 stream clients, which means, we'll have 5 clients got closed. And each closed client need 45 sec to trigger rebalance, so we'll have 45 * 5 = 225 sec (~4 mins) of the time having some tasks not working. For (2), during new client created and old client closed, it need some time to do rebalance. With 45 session timeout, we only got ~100 sec left. In slow jenkins env, it might reach the 30 retries without getting any data timeout. Therefore, decreasing session timeout can make this test completes faster and more reliable. Reviewers: Guozhang Wang <wangguoz@gmail.com> * MINOR: enable EOS during smoke test IT (apache#10870) This IT has been failing on trunk recently. Enabling EOS during the integration test makes it easier to be sure that the test's assumptions are really true during verification and should make the test more reliable. I also noticed that in the actual system test file, we are using the deprecated property name "beta" instead of "v2". Reviewers: Boyang Chen <boyang@apache.org> * MINOR: Log formatting for exceptions during configuration related operations (apache#10843) Format configuration logging during exceptions or errors. Also make sure it redacts sensitive information or unknown values. Reviewers: Luke Chen <showuon@gmail.com>, David Jacot <djacot@confluent.io> * KAFKA-12914: StreamSourceNode should return `null` topic name for pattern subscription (apache#10846) Reviewers: Luke Chen <showuon@gmail.com>, Bruno Cadonna <bruno@confluent.io>, Guozhang Wang <guozhang@confluent.io> * KAFKA-12948: Remove node from ClusterConnectionStates.connectingNodes when node is removed (apache#10882) NetworkClient.poll() throws IllegalStateException when checking isConnectionSetupTimeout if all nodes in ClusterConnectionStates.connectingNodes aren't present in ClusterConnectionStates.nodeState. This commit ensures that when we remove a node from nodeState, we also remove from connectingNodes. Reviewers: David Jacot <djacot@confluent.io> * KAFKA-12701: NPE in MetadataRequest when using topic IDs (apache#10584) We prevent handling MetadataRequests where the topic name is null (to prevent NPE) as well as prevent requests that set topic IDs since this functionality has not yet been implemented. When we do implement it in apache#9769, we should bump the request/response version. Added tests to ensure the error is thrown. Reviewers: dengziming <swzmdeng@163.com>, Ismael Juma <ismael@juma.me.uk> Co-authored-by: Josep Prat <josep.prat@aiven.io> Co-authored-by: Ismael Juma <ismael@juma.me.uk> Co-authored-by: David Christle <dchristle@users.noreply.github.com> Co-authored-by: Luke Chen <showuon@gmail.com> Co-authored-by: John Roesler <vvcephei@users.noreply.github.com> Co-authored-by: YiDing-Duke <dingyi.zj@gmail.com> Co-authored-by: Rajini Sivaram <rajinisivaram@googlemail.com> Co-authored-by: Justine Olshan <jolshan@confluent.io>

KAFKA-12914: StreamSourceNode should return null topic name for pat…

835133f

…tern subscription

showuon approved these changes Jun 9, 2021

View reviewed changes

cadonna approved these changes Jun 10, 2021

View reviewed changes

Change return type to Optional

006560f

guozhangwang approved these changes Jun 13, 2021

View reviewed changes

mjsax merged commit 01967e4 into apache:trunk Jun 15, 2021

mjsax deleted the kafka-12914-fix-npe branch June 15, 2021 07:59

mjsax added a commit that referenced this pull request Jun 15, 2021

KAFKA-12914: StreamSourceNode should return null topic name for pat…

b748340

…tern subscription (#10846) Reviewers: Luke Chen <showuon@gmail.com>, Bruno Cadonna <bruno@confluent.io>, Guozhang Wang <guozhang@confluent.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-12914: StreamSourceNode should return `null` topic name for pattern subscription#10846

KAFKA-12914: StreamSourceNode should return `null` topic name for pattern subscription#10846
mjsax merged 2 commits intoapache:trunkfrom
mjsax:kafka-12914-fix-npe

mjsax commented Jun 8, 2021

Uh oh!

showuon left a comment

Uh oh!

mjsax commented Jun 9, 2021

Uh oh!

showuon commented Jun 9, 2021

Uh oh!

cadonna commented Jun 9, 2021

Uh oh!

mjsax commented Jun 9, 2021

Uh oh!

cadonna left a comment

Uh oh!

mjsax commented Jun 11, 2021

Uh oh!

guozhangwang left a comment

Uh oh!

mjsax commented Jun 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

mjsax commented Jun 8, 2021

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax commented Jun 9, 2021

Uh oh!

showuon commented Jun 9, 2021

Uh oh!

cadonna commented Jun 9, 2021

Uh oh!

mjsax commented Jun 9, 2021

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax commented Jun 11, 2021

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax commented Jun 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments