Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][client] ExtNonPersistentTopics and prevent prefix match #19065

Merged
merged 2 commits into from
Dec 27, 2022

Conversation

tisonkun
Copy link
Member

This is a follow-up to #19025. That the swagger plugin is matching by prefix so we have to distinguish the class name in prefix.

Also add back the inherit logic for NonPersistentTopics and fix a bug inline.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository:

@tisonkun tisonkun requested review from tuteng and yuruguo December 26, 2022 14:54
@tisonkun tisonkun self-assigned this Dec 26, 2022
@github-actions github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Dec 26, 2022
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Member Author

Verified with mvn -X -ntp clean install -Pcore-modules,swagger,-main -DskipTests -DskipSourceReleaseAssembly=true -Dspotbugs.skip=true -Dlicense.skip=true and ensure that these two Ext classes is not counted (while previous it is).

try {
validateNamespaceName(tenant, namespace);
validateGlobalNamespaceOwnership();
validateTopicName(tenant, namespace, encodedTopic);
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is a bug introduced in #14117. That NonPersistentTopics inherits PersistentTopics but this endpoint should override:

image

cc @lhotari

@codecov-commenter
Copy link

codecov-commenter commented Dec 26, 2022

Codecov Report

Merging #19065 (c1c3969) into master (b42aed1) will increase coverage by 0.26%.
The diff coverage is 16.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19065      +/-   ##
============================================
+ Coverage     47.11%   47.37%   +0.26%     
- Complexity    10595    10711     +116     
============================================
  Files           710      711       +1     
  Lines         69423    69454      +31     
  Branches       7449     7452       +3     
============================================
+ Hits          32709    32907     +198     
+ Misses        33037    32877     -160     
+ Partials       3677     3670       -7     
Flag Coverage Δ
unittests 47.37% <16.66%> (+0.26%) ⬆️

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

Impacted Files Coverage Δ
...pulsar/broker/admin/v2/ExtNonPersistentTopics.java 0.00% <0.00%> (ø)
...n/java/org/apache/pulsar/broker/PulsarService.java 57.60% <33.33%> (-0.45%) ⬇️
...he/pulsar/broker/admin/v2/ExtPersistentTopics.java 76.92% <100.00%> (ø)
...rvice/schema/KeyValueSchemaCompatibilityCheck.java 21.62% <0.00%> (-45.95%) ⬇️
...apache/pulsar/broker/service/TopicListService.java 10.40% <0.00%> (-30.40%) ⬇️
...g/apache/bookkeeper/mledger/util/StatsBuckets.java 43.75% <0.00%> (-16.67%) ⬇️
...pulsar/broker/service/PulsarCommandSenderImpl.java 59.18% <0.00%> (-12.76%) ⬇️
...oker/service/nonpersistent/NonPersistentTopic.java 38.93% <0.00%> (-12.53%) ⬇️
...ookkeeper/mledger/impl/ManagedLedgerMBeanImpl.java 53.17% <0.00%> (-9.53%) ⬇️
...ce/schema/validator/StructSchemaDataValidator.java 52.38% <0.00%> (-9.53%) ⬇️
... and 44 more

Copy link
Contributor

@yuruguo yuruguo left a comment

Choose a reason for hiding this comment

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

LGTM

@tisonkun
Copy link
Member Author

Merging...

@tisonkun tisonkun merged commit 4f72fdb into apache:master Dec 27, 2022
@tisonkun tisonkun deleted the fix-prefix-match branch December 27, 2022 01:55
tisonkun added a commit to tisonkun/pulsar that referenced this pull request Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants