-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[improve][admin]internalGetMessageById shouldn't be allowed on partitioned topic #19013
Conversation
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #19013 +/- ##
============================================
- Coverage 47.73% 47.67% -0.06%
- Complexity 10819 10825 +6
============================================
Files 712 712
Lines 69645 69650 +5
Branches 7481 7480 -1
============================================
- Hits 33242 33203 -39
- Misses 32699 32758 +59
+ Partials 3704 3689 -15
Flags with carried forward coverage won't be shown. Click here to find out more.
|
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please add the test for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall right, and it is better if you can add a unit test to verify it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please add the test for it?
+1
eddad52
to
73a5586
Compare
org.apache.pulsar.broker.admin.PersistentTopicsTest#testGetMessageById |
Motivation
Now, if a topic without partition index provided when querying
internalGetMessageById
, errorTopic not found
will be thrown out. It will confuse users.We'd better forbidden
internalGetMessageById
on partitioned-topic.Modifications
If a partitioned-topic is provided,
Status.METHOD_NOT_ALLOWED
will be thrown outVerifying this change
(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:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
gaozhangmin#5