-
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
[feat][broker]PIP-180 ShadowTopic - Part III - Add shadowTopics in TopicPolicy #17242
Conversation
/pulsarbot run-failure-checks |
a41a585
to
cfa37a4
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 lgtm
I have a concern about the name of the REST endpoint
@@ -4236,5 +4236,84 @@ public void setSchemaValidationEnforced(@Suspended AsyncResponse asyncResponse, | |||
}); | |||
} | |||
|
|||
@GET | |||
@Path("/{tenant}/{namespace}/{topic}/shadowTopic") |
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.
Should the name be 'plural' ? shadowTopics
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.
@eolivelli Make sense, addressed.
LGTM |
update REST endpoints fix test
@eolivelli PTAL |
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.
LGTM
Master Issue: #16153
Motivation
Source topic need to know its shadow topic list to start the ShadowReplicators.
Note: Shadow topic also need to know its source topic, but it can't implemented with topic policy. Because topic policy can only guarantee eventual consistency and shadow topic must know its source topic during initialization.
Modifications
Add shadowTopics in TopicPolicy and create/delete/get commands in admin clients.
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation
Check the box below or label this PR directly.
Need to update docs?
doc-required
(Your PR needs to update docs and you will update later)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
Doc added in pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Topics.java