Skip to content
This repository has been archived by the owner on Jan 24, 2024. It is now read-only.

Fix wrong semantics and usage of KafkaTopicManager#getTopic #473

Merged

Conversation

BewareMyPower
Copy link
Collaborator

@BewareMyPower BewareMyPower commented May 7, 2021

Motivation

Many usages of KafkaTopicManager#getTopic are wrong because they didn't check null value so that NPE may happen. And the semantics of KafkaTopicManager#getTopic is wrong because the createIfMissing argument of BrokerService#getTopic is true, so if a not existed topic's name was accepted as the argument, the topic would be created automatically. This behavior is unexpected because we only want to create topics automatically in METADATA request handler.

Modifications

  1. In KafkaTopicManager#getTopic, when BrokerService#getTopic is completed exceptionally, returns an exceptionally completed future instead of a null completed future.
  2. Use whenComplete for all getTopic references so that we can handle null completed futures and exceptionally completed futures.
  3. The LIST_OFFSET request handler is modified according to the above changes
    • For exceptionally completed getTopic future, return the thrown exception.
    • For null completed getTopic future, return the UNKNOWN_TOPIC_OR_PARTITION error.
  4. Add tests for listing offsets of a not existed topic. Since the default ctx of KafkaRequestHandler is null and may cause NPE during error logging, we also set a mocked ChannelHandlerContext in the test.
  5. Remove redundant NPE warning logs in MessageFetchContext#handleFetch since we've already added logs to the case that the future of KafkaTopicConsumerManager is completed exceptionally.

@BewareMyPower
Copy link
Collaborator Author

There're some failing tests that need to be fixed:

Error:    KafkaTopicConsumerManagerTest.testGetTopicConsumerManager:115 expected [true] but found [false]
Error:    KafkaTopicConsumerManagerTest.testTopicConsumerManagerRemoveAndAdd:145 NullPointer

@BewareMyPower BewareMyPower force-pushed the bewaremypower/handle-null-get-topic branch from 471ab63 to de5702e Compare May 8, 2021 07:42
@BewareMyPower BewareMyPower changed the title [WIP] Fix wrong semantics and usage of KafkaTopicManager#getTopic Fix wrong semantics and usage of KafkaTopicManager#getTopic May 8, 2021
@BewareMyPower BewareMyPower force-pushed the bewaremypower/handle-null-get-topic branch from de5702e to b385e2a Compare May 8, 2021 08:30
@BewareMyPower BewareMyPower changed the title Fix wrong semantics and usage of KafkaTopicManager#getTopic [WIP] Fix wrong semantics and usage of KafkaTopicManager#getTopic May 8, 2021
@BewareMyPower BewareMyPower changed the title [WIP] Fix wrong semantics and usage of KafkaTopicManager#getTopic Fix wrong semantics and usage of KafkaTopicManager#getTopic May 8, 2021
@BewareMyPower
Copy link
Collaborator Author

@jiazhai now this PR is ready for review, PTAL.

@jiazhai jiazhai merged commit a310c7e into streamnative:master May 9, 2021
@BewareMyPower BewareMyPower deleted the bewaremypower/handle-null-get-topic branch May 9, 2021 16:15
jiazhai pushed a commit that referenced this pull request May 14, 2021
Fixes #493

This bug was introduced by #473. In `MessageFetchContext#handleFetch`, when the `KafkaTopicConsumerManager`'s future is completed with null, we should remove the future from `KafkaTopicManager#consumerTopicManagers`.

In addition, this PR adds some refactors for `consumerTopicManagers`:
1. Don't use getter to expose this field, use methods to operate it instead.
2. Check null for completed future before close `KafkaTopicConsumerManager`.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants