Skip to content

Conversation

@mattisonchao
Copy link
Member

Motivation

This behaviour is broken by PR #18193,

In the previous version, we could use the pulsar client to look up the topic with the -partition- keyword. Even this topic doesn't have partition metadata. But currently, we can't do that. You can check the test to know the detail.

Modifications

  • Allow user lookup topic name with -partition- but no metadata

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • doc-not-needed

@mattisonchao mattisonchao self-assigned this Jan 10, 2023
@mattisonchao mattisonchao requested review from Jason918, Technoboy-, codelipenghui, dlg99 and eolivelli and removed request for dlg99 and eolivelli January 10, 2023 11:28
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 10, 2023
@mattisonchao mattisonchao added this to the 2.12.0 milestone Jan 10, 2023
@mattisonchao mattisonchao reopened this Jan 10, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2023

Codecov Report

Merging #19171 (391ed59) into master (b05fddb) will increase coverage by 1.78%.
The diff coverage is 77.77%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19171      +/-   ##
============================================
+ Coverage     45.64%   47.42%   +1.78%     
+ Complexity    11043    10779     -264     
============================================
  Files           773      713      -60     
  Lines         74463    69730    -4733     
  Branches       8018     7496     -522     
============================================
- Hits          33986    33072     -914     
+ Misses        36687    32933    -3754     
+ Partials       3790     3725      -65     
Flag Coverage Δ
unittests 47.42% <77.77%> (+1.78%) ⬆️

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

Impacted Files Coverage Δ
...ache/pulsar/broker/namespace/NamespaceService.java 57.91% <71.42%> (-0.17%) ⬇️
.../apache/pulsar/broker/admin/impl/ClustersBase.java 76.68% <81.81%> (+0.02%) ⬆️
...ar/broker/loadbalance/impl/BundleSplitterTask.java 54.00% <0.00%> (-28.00%) ⬇️
.../pulsar/broker/service/SharedConsumerAssignor.java 61.11% <0.00%> (-16.67%) ⬇️
...er/loadbalance/impl/ModularLoadManagerWrapper.java 73.91% <0.00%> (-6.53%) ⬇️
...roker/loadbalance/impl/ModularLoadManagerImpl.java 58.97% <0.00%> (-6.08%) ⬇️
.../pulsar/broker/service/BrokerServiceException.java 45.45% <0.00%> (-5.46%) ⬇️
...he/pulsar/client/impl/PartitionedProducerImpl.java 30.34% <0.00%> (-5.13%) ⬇️
...r/service/AbstractDispatcherMultipleConsumers.java 66.12% <0.00%> (-4.84%) ⬇️
...ervice/streamingdispatch/StreamingEntryReader.java 57.89% <0.00%> (-3.51%) ⬇️
... and 96 more

Copy link
Contributor

@poorbarcode poorbarcode left a comment

Choose a reason for hiding this comment

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

LGTM

@eolivelli eolivelli merged commit a93f30f into apache:master Jan 12, 2023
@mattisonchao mattisonchao deleted the fix_breaking_change branch January 12, 2023 09:21
.create();
producer.close();
// Check the topic exist in the list again.
Assert.assertTrue(topics.contains(partition2));
Copy link
Contributor

Choose a reason for hiding this comment

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

we didn't call topics = admin.topics().getList("prop/ns-abc"); again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants