Skip to content

Conversation

@fxbing
Copy link
Contributor

@fxbing fxbing commented Aug 15, 2019

Master Issue: #4926

Motivation

Curently the partitioned-topic and non-partitioned topic is a little confuse for users. in PR #3450 we add config for auto-topic-creation.
We could leverage this config to provide some more config for auto-topic-creation.

Modifications

  • Add allowAutoTopicCreationType and allowAutoTopicCreationNumPartitions to configuration.
  • Users can use both configurations when they decide to create a topic automatically.
  • Add test.
  • Update doc.

Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fxbing it is a nice motivation. but I am feeling that you didn't change the code in the right place. the change in fetchPartitionedTopicMetadataAsync will introduce unnecessary creation of partitioned topic. The auto-creation happens when a producer or a consumer connects to broker. so I think the change should be done in BrokerService. You can check the methods getOrCreateTopic.

Also change "partition" and "non-partition" to "partitioned" and "non-partitioned". It might be also worth considering adding an enum like TopicType?

@fxbing
Copy link
Contributor Author

fxbing commented Aug 16, 2019

@fxbing it is a nice motivation. but I am feeling that you didn't change the code in the right place. the change in fetchPartitionedTopicMetadataAsync will introduce unnecessary creation of partitioned topic. The auto-creation happens when a producer or a consumer connects to broker. so I think the change should be done in BrokerService. You can check the methods getOrCreateTopic.

I first thought about making changes in the BrokerService, but when the client requests to create a Producer or Consumer, it will first get partitions info from zk, decide whether to create partitioned or non-partitioned. When the request arrives at the BrokerService, the client has decided to create a nonpartitioned Producer or Consumer. If the Broker's configuration requires automatic creation of a partitioned topic, Broker needs to send the configuration information to client, client re-establishes the partitioned Producer or Consumer, and the BrokerService updates the partitions in the zk.

I think it's easier to make changes when the client creates the type that determines the creation, and need fewer communications.

Do I need to modify it as I said above?

@fxbing
Copy link
Contributor Author

fxbing commented Aug 16, 2019

Also change "partition" and "non-partition" to "partitioned" and "non-partitioned". It might be also worth considering adding an enum like TopicType?

OK, I will make a change.

@fxbing
Copy link
Contributor Author

fxbing commented Aug 17, 2019

run java8 tests

@fxbing
Copy link
Contributor Author

fxbing commented Aug 18, 2019

run Integration Tests

Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fxbing well done. I have two questions to clarify with you and @jiazhai

the first one is about the setting name about the number of partitions.
the second one is whether we need to check if a non-partitioned topic exists or not when the partitioned-topic doesn't exist.

PTAL

@sijie sijie added area/admin area/broker type/feature The PR added a new feature or issue requested a new feature labels Aug 18, 2019
@sijie sijie added this to the 2.5.0 milestone Aug 18, 2019
@fxbing
Copy link
Contributor Author

fxbing commented Aug 20, 2019

run C++ / Python Tests
run Integration Tests

@fxbing
Copy link
Contributor Author

fxbing commented Aug 20, 2019

run cpp tests

@fxbing fxbing force-pushed the add-config-for-auto-topic-creation branch 2 times, most recently from c4382da to de35875 Compare August 22, 2019 03:32
@fxbing
Copy link
Contributor Author

fxbing commented Aug 22, 2019

run cpp tests

@fxbing
Copy link
Contributor Author

fxbing commented Aug 22, 2019

run cpp tests
run integration tests
run java8 tests

2 similar comments
@fxbing
Copy link
Contributor Author

fxbing commented Aug 22, 2019

run cpp tests
run integration tests
run java8 tests

@fxbing
Copy link
Contributor Author

fxbing commented Aug 25, 2019

run cpp tests
run integration tests
run java8 tests

@fxbing fxbing force-pushed the add-config-for-auto-topic-creation branch 5 times, most recently from 580d8a7 to a068ba3 Compare August 28, 2019 06:45
@fxbing fxbing force-pushed the add-config-for-auto-topic-creation branch from c2e0629 to fd5da63 Compare September 11, 2019 12:24
@fxbing
Copy link
Contributor Author

fxbing commented Sep 11, 2019

run integration tests

@fxbing
Copy link
Contributor Author

fxbing commented Sep 12, 2019

run java8 tests

@fxbing
Copy link
Contributor Author

fxbing commented Sep 12, 2019

PTAL @sijie @jiazhai @codelipenghui

Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fxbing Nice work!
Overall looks good to me, just left a suggestion

return metadataFuture;
}

protected static CompletableFuture<PartitionedTopicMetadata> createDefaultPartitionedTopicAsync(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this method is no longer an asynchronous method, it's better to rename it to createDefaultPartitionedTopic()

@sijie sijie merged commit 547c421 into apache:master Sep 17, 2019
@sijie
Copy link
Member

sijie commented Sep 17, 2019

@jiazhai @jennifer88huang can you please make sure this feature is well documented?

@Jennifer88huang-zz
Copy link
Contributor

Got it.

wolfstudy pushed a commit that referenced this pull request Nov 20, 2019
Master Issue:  #4926

Curently the partitioned-topic and non-partitioned topic is a little confuse for users. in PR #3450 we add config for auto-topic-creation.
We could leverage this config to provide some more config for auto-topic-creation.

- Add `allowAutoTopicCreationType` and `allowAutoTopicCreationNumPartitions` to configuration.
- Users can use both configurations when they decide to create a topic automatically.
- Add test.
- Update doc.

(cherry picked from commit 547c421)
@wolfstudy wolfstudy modified the milestones: 2.5.0, 2.4.2 Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/admin area/broker type/feature The PR added a new feature or issue requested a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants