-
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][pip] PIP-398: Subscription replication on the broker, namespace and topic levels #23770
base: master
Are you sure you want to change the base?
[improve][pip] PIP-398: Subscription replication on the broker, namespace and topic levels #23770
Conversation
#23769 aims to overwrite the subscription replication state from the consumer, which is different from this PIP. Actually, I also need an overwrite feature, and then the users use the admin API to manage the subscription replication. The downstream users sometimes enable/disable the subscription replication on the consumer, this lead to the namespace or topic level is ignored. We can also merge #23769 to this PIP. What's your opinion? @lhotari @yyj8 |
@nodece @yyj8 Please work together to combine both cases since they are both valuable and useful. I think it would make |
This is a great idea and suggestion. Can we consider a strategy that divides into three dimensions: cluster dimension, namespace dimension, and topic dimension. |
Once the consumer level is configured, it is the highest priority, the cluster, namespace, and topic levels will be ignored, please see #23769 (comment) for details.
This is feasible. |
The default configuration for the client is My suggestion is to configure the cluster dimension. If |
The latest client defaults to
I understand your idea, this can make all subscriptions replicated when However, this idea conflicts with PIP-398, which assumes that the consumer-level configuration for In our case, I don't want to configure the My ideaTo combine your case and my case, I suggest introducing a broker configuration for overwriting the consume configuration, and assuming the namespace and topic level are set, the final result will be so like this:
We can also introduce |
73a4da4
to
4e7ee76
Compare
@nodece I agree with your suggestion. Then we can make unified code adjustments on your pip, or you can directly merge my code into your code, or after you submit the code, I can synchronize your code and modify the cluster level configuration before submitting. |
@yyj8 Correct. The Breaking Change: For an existing subscription where The new subscription will follow the above priority. /cc @lhotari |
cd4d790
to
c421642
Compare
@yyj8 This PIP has been updated, could you have a chance to review this PIP? |
Are you referring to updating the code and modifying the functional design specifications in this pip pip/pip-398.md design documentation? |
Just for this PIP. |
… topic levels Signed-off-by: Zixuan Liu <[email protected]>
Signed-off-by: Zixuan Liu <[email protected]>
c421642
to
9a493a9
Compare
Signed-off-by: Zixuan Liu <[email protected]>
9a493a9
to
b335e5b
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.
Added some thoughts.
##### Namespace level | ||
|
||
- `/{tenant}/{namespace}/replicateSubscriptionState`: enable/disable/remove the subscription replication on the | ||
namespace level. | ||
- Method: `POST` | ||
- Content-Type: `application/json` | ||
- Body: | ||
- true | ||
- false | ||
- null | ||
- `GET /{tenant}/{namespace}/replicateSubscriptionState` to get subscription replication configuration on the namespace | ||
level. | ||
- Method: `GET` | ||
- Response: | ||
- true | ||
- false | ||
- null |
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.
Just wondering if this would be more of a namespace policy matter at the namespace level
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.
Yes.
##### Topic level | ||
|
||
- `/{tenant}/{namespace}/{topic}/replicateSubscriptionState`: enable/disable/remove the subscription replication on the | ||
topic level. | ||
- Method: `POST` | ||
- Content-Type: `application/json` | ||
- Body: | ||
- true | ||
- false | ||
- null | ||
- `/{tenant}/{namespace}/{topic}/replicateSubscriptionState` to get subscription replication configuration on the topic | ||
level. | ||
- Method: `GET` | ||
- Parameters: | ||
- `applied=true`: get the applied subscription replication configuration, if topic is not set, return the | ||
namespace level configuration. | ||
- `applied=false`: get the applied subscription replication configuration, if topic is set, return the topic | ||
level configuration. | ||
- Response: | ||
- true | ||
- false | ||
- null |
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 feels that this should be part of the topic policy.
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.
Yes.
- If `replicateSubscriptionState` is set at the consumer level, configurations at the topic, namespace, and broker levels are | ||
ignored. | ||
- If set at the topic level, the namespace-level configuration is ignored. | ||
- If set at the namespace level, the broker-level configuration is ignored. |
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.
I think that there could be a need to override any value set on the consumer side. I also added some comments that controlling the behavior at namespace and consumer level should perhaps be a policy concern instead of creating a separate concept for handling the setting.
If the setting at namespace and topic level would be more than just a true/false. It would be possible to add a separate setting that would allow overriding the consumer level setting.
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.
I think that there could be a need to override any value set on the consumer side.
It's also possible.
I also added some comments that controlling the behavior at namespace and consumer level should perhaps be a policy concern instead of creating a separate concept for handling the setting.
Yes. This config is present in the broker/namespace policies/topic policies/cursor property.
If the setting at namespace and topic level would be more than just a true/false. It would be possible to add a separate setting that would allow overriding the consumer level setting.
I don't want to introduce more config, so when the consume level is false, the broker/namespace policies/topic policies can override the consume level setting.
The term |
Signed-off-by: Zixuan Liu <[email protected]>
"replicating the subscription state" is correct. This term is from the design of existing consumer. |
@lhotari Do you have more suggestions? If not, I will send a vote to the mailing list. |
@nodece I think that at the topic and namespace level, it could be better to combine
I don't know if this is accurate, but putting more thought in how to address the topic and namespace level would be useful. I haven't seen the APIs being updated to have this information in the policies. I don't think that introducing a completely separate concept for topic and namespace level makes sense. |
This is an important section: https://github.com/apache/pulsar/pull/23770/files#diff-67fc7a48cc071911c2239d1c628335d4147f54345051f36efd2f18dbcc8339c6R110 Since the consumer enables the subscription replication when the Prioritizing Another thing is that we did not persist the case where
The |
There is a special case here, if the user intentionally sets `false` at the consumer level, and then set `true` at the | ||
topic/ns/broker level, this may disrupt the user's behavior. This way, we can minimize changes to the Pulsar public API | ||
as much as possible. |
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 would be good to explain what "disrupt the user's behavior" means. Perhaps rewording with the help of Generative AI could be useful since "disruption" is typically used in a different context.
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 user expected to disable subscription replication, however, found that the feature had been enabled.
@nodece Yes, I understand that. Isn't there a need to override any client application consumer level setting with topic or namespace level policies? What if Pulsar administrator would explicitly want to disallow or force using replicated subscriptions for a particular topic or namespace while geo-replication is enabled? |
When the topic/namespace level is true, the consumer level will be ignored. Usually, we want to enable the subscription replication, not disable.
The administrator is permitted to enable the subscription replication and subsequently disable it. |
Motivation
Enhance the subscription replication.
Documentation
doc
doc-required
doc-not-needed
doc-complete