Skip to content

[fix][broker] Support deleting partitioned topics with the keyword -partition-#19230

Merged
mattisonchao merged 1 commit intoapache:masterfrom
mattisonchao:support_delete_topic_with_keyword
Jan 14, 2023
Merged

[fix][broker] Support deleting partitioned topics with the keyword -partition-#19230
mattisonchao merged 1 commit intoapache:masterfrom
mattisonchao:support_delete_topic_with_keyword

Conversation

@mattisonchao
Copy link
Copy Markdown
Member

Motivation

We allow users to use the client to create the partitioned topic when they enable partitioned type auto-creation. But we didn't support deleting it.

Modifications

  • Support deleting partitioned topics with the keyword -partition-

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@mattisonchao mattisonchao self-assigned this Jan 13, 2023
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jan 13, 2023
@mattisonchao mattisonchao reopened this Jan 13, 2023
@mattisonchao mattisonchao added this to the 2.12.0 milestone Jan 13, 2023
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 13, 2023

Codecov Report

Merging #19230 (4313e88) into master (b05fddb) will increase coverage by 2.35%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19230      +/-   ##
============================================
+ Coverage     45.64%   47.99%   +2.35%     
+ Complexity    11043    10896     -147     
============================================
  Files           773      713      -60     
  Lines         74463    69724    -4739     
  Branches       8018     7497     -521     
============================================
- Hits          33986    33464     -522     
+ Misses        36687    32555    -4132     
+ Partials       3790     3705      -85     
Flag Coverage Δ
unittests 47.99% <83.33%> (+2.35%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../apache/pulsar/broker/admin/impl/ClustersBase.java 76.68% <81.81%> (+0.02%) ⬆️
...pache/pulsar/broker/admin/v2/PersistentTopics.java 74.80% <100.00%> (ø)
.../pulsar/broker/service/SharedConsumerAssignor.java 68.51% <0.00%> (-9.26%) ⬇️
...r/client/impl/schema/reader/JacksonJsonReader.java 66.66% <0.00%> (-6.67%) ⬇️
...sar/broker/service/schema/SchemaRegistryStats.java 71.25% <0.00%> (-6.25%) ⬇️
...g/apache/pulsar/client/impl/ConnectionHandler.java 50.00% <0.00%> (-5.32%) ⬇️
...he/pulsar/client/impl/PartitionedProducerImpl.java 30.34% <0.00%> (-5.13%) ⬇️
...ervice/streamingdispatch/StreamingEntryReader.java 56.72% <0.00%> (-4.68%) ⬇️
...ker/resourcegroup/ResourceGroupConfigListener.java 64.38% <0.00%> (-2.74%) ⬇️
...oker/service/schema/SchemaRegistryServiceImpl.java 63.29% <0.00%> (-2.03%) ⬇️
... and 129 more

@mattisonchao mattisonchao merged commit fc4bca6 into apache:master Jan 14, 2023
@mattisonchao mattisonchao deleted the support_delete_topic_with_keyword branch January 14, 2023 03:55
@yuruguo
Copy link
Copy Markdown
Contributor

yuruguo commented Jan 14, 2023

  1. Whether the deletTopic can delete topic with the keyword -partition- ?

  2. The root cause of this problem is that the conditions for creating partitioned topic by the Admin API and the Client API are inconsistent, as below:
    (1) Admin API
    L265

    if (encodedTopic.contains(TopicName.PARTITIONED_TOPIC_SUFFIX)) {
    throw new RestException(Status.PRECONDITION_FAILED,
    "Partitioned Topic Name should not contain '-partition-'");
    }

    It is introduce in Issue 1067: Problems with Partitioned Topics which name contains -partition-N #2342
    (2) Client API
    L2941
    if (metadata.partitions == 0
    && !topicExists
    && !topicName.isPartitioned()
    && pulsar.getBrokerService()
    .isDefaultTopicTypePartitioned(topicName, policies)) {
    isAllowAutoTopicCreationAsync(topicName, policies).thenAccept(allowed -> {

    It is introduce in Avoid creating partitioned topic for partition name #6846
    So I think we should not modify the delete operation (revert this PR), then keep the behavior consistent for creating partitioned topic by theAdmin API and the Client API (create a new PR).

cc @mattisonchao @codelipenghui @coderzc

@mattisonchao
Copy link
Copy Markdown
Member Author

Whether the deletTopic can delete topic with the keyword -partition- ?

Please take a look at the test. We not only need to delete the topic but also topic metadata.

The root cause of this problem is that the conditions for creating partitioned topic by the Admin API and the Client API are inconsistent, as below:

Yes, that is the root cause, but we should consider the compatibility for some users using current behaviour.
e.g., they have created this kind of topic but can't delete it.

It is introduce in #6846

We didn't limit all the cases.
e.g. persistent://public/default/aaa-partition-bbb

This topic name can still create partitioned topic metadata.

So I think we should not modify the delete operation (revert this PR), then keep the behavior consistent for creating partitioned topic by theAdmin API and the Client API (create a new PR).

If you have any good way to solve the problem I mentioned, I would be pleased to revert the current PR.

mattisonchao added a commit that referenced this pull request Jan 15, 2023
…19234)

### Motivation
This PR is following up #19230

As @yuruguo mentioned #19230 (comment), we can tighten the validation scope.

I've checked the logic and found we have no way to create the partition topic with the `-partition-{index}` template. So we can righten the validation scope.

I will keep working on the partition topic section and try to clarify the concept and logic. Plus, ensuring compatibility.

### Modifications

- tighten the validation scope
mattisonchao added a commit that referenced this pull request Jan 16, 2023
mattisonchao added a commit that referenced this pull request Jan 16, 2023
…19234)

### Motivation
This PR is following up #19230

As @yuruguo mentioned #19230 (comment), we can tighten the validation scope.

I've checked the logic and found we have no way to create the partition topic with the `-partition-{index}` template. So we can righten the validation scope.

I will keep working on the partition topic section and try to clarify the concept and logic. Plus, ensuring compatibility.

### Modifications

- tighten the validation scope

(cherry picked from commit 246c270)
mattisonchao added a commit that referenced this pull request Jan 16, 2023
mattisonchao added a commit that referenced this pull request Jan 16, 2023
mattisonchao added a commit that referenced this pull request Jan 17, 2023
…19234)

### Motivation
This PR is following up #19230

As @yuruguo mentioned #19230 (comment), we can tighten the validation scope.

I've checked the logic and found we have no way to create the partition topic with the `-partition-{index}` template. So we can righten the validation scope.

I will keep working on the partition topic section and try to clarify the concept and logic. Plus, ensuring compatibility.

### Modifications

- tighten the validation scope

(cherry picked from commit 246c270)
liangyepianzhou pushed a commit that referenced this pull request Feb 6, 2023
liangyepianzhou pushed a commit that referenced this pull request Feb 25, 2023
…19234)

This PR is following up #19230

As @yuruguo mentioned #19230 (comment), we can tighten the validation scope.

I've checked the logic and found we have no way to create the partition topic with the `-partition-{index}` template. So we can righten the validation scope.

I will keep working on the partition topic section and try to clarify the concept and logic. Plus, ensuring compatibility.

- tighten the validation scope

(cherry picked from commit 246c270)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Feb 28, 2023
…partition-` (apache#19230)

(cherry picked from commit fc4bca6)
(cherry picked from commit 0ade728)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Feb 28, 2023
…ope (apache#19234)

This PR is following up apache#19230

As @yuruguo mentioned apache#19230 (comment), we can tighten the validation scope.

I've checked the logic and found we have no way to create the partition topic with the `-partition-{index}` template. So we can righten the validation scope.

I will keep working on the partition topic section and try to clarify the concept and logic. Plus, ensuring compatibility.

- tighten the validation scope

(cherry picked from commit 246c270)
(cherry picked from commit 2e3e9de)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants