-
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
Converted to v2 topic names test related to ProducerConsumerBase #1562
Conversation
3ca7308
to
085139d
Compare
085139d
to
2dfed20
Compare
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 looks good to me. left some comments.
@@ -128,7 +128,7 @@ public void deleteNamespace(@PathParam("property") String property, @PathParam(" | |||
} | |||
|
|||
@DELETE | |||
@Path("/{property}/{namespace}/bundle/{bundle}") |
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.
hmm this sounds like a breaking change, no?
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.
This is only relative to v2 namespace. It was broken when v2 handler created.
import org.apache.pulsar.broker.web.NoSwaggerDocumentation; | ||
import org.apache.pulsar.common.naming.TopicName; | ||
|
||
@Path("/v2/destination/") |
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.
the path here is wrong?
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.
It's probably worth a comment in the code, but it's correct.
The lookup API was already /v2/
in Pulsar 1.xxx. This was internally versioned at Yahoo to not clash from earlier API.
Since we're adding now the "Pulsar v2" we cannot rename this topic lookup into /v1
. Rather the difference here would be : lookup/v2/destination/persistent/prop/cluster/ns/topic
vs lookup/v2/topic/persistent/prop/ns/topic
.
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.
awesome!
Motivation
This is the first set of changes to convert unit tests to use v2 topic names.
The PR contains also fixes, mainly to v2 lookup APIs to make sure all the functionalities were supported.
This first batch started the conversion from
ProducerConsumerBase
test class and all the tests that extends from that. AV1_ProducerConsumerTest
test has been retained with v1 name to ensure we don't break existing topics.