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

Polices On Topic #2688

Closed
14 tasks done
codelipenghui opened this issue Oct 1, 2018 · 13 comments
Closed
14 tasks done

Polices On Topic #2688

codelipenghui opened this issue Oct 1, 2018 · 13 comments
Assignees
Labels
area/broker area/cli type/feature The PR added a new feature or issue requested a new feature
Milestone

Comments

@codelipenghui
Copy link
Contributor

codelipenghui commented Oct 1, 2018

This is the master ticket for tracking all the tasks for polices on topic

@sijie sijie added this to the 2.3.0 milestone Oct 1, 2018
@sijie sijie added type/feature The PR added a new feature or issue requested a new feature triage/week-40 and removed triage/week-39 labels Oct 1, 2018
@ivankelly
Copy link
Contributor

@codelipenghui

This change will cause more load on zookeeper. For each topic loaded, we would have to do a read for the policy of that topic when loading the topic policy, even if no topic policy exists.

How does the broker get notified of updated to the policy for a topic? If we add a watcher, that's a watcher per topic. We should consider if there's another way of doing this without placing more load on ZK, such as in a 'system' topic.

@codelipenghui
Copy link
Contributor Author

@ivankelly

This change will cause more load on zookeeper. For each topic loaded, we would have to do a read for the policy of that topic when loading the topic policy, even if no topic policy exists.

How does the broker get notified of updated to the policy for a topic? If we add a watcher, that's a watcher per topic. We should consider if there's another way of doing this without placing more load on ZK, such as in a 'system' topic.

Yes, this is a very necessary concern. I think 'system' topic is an excellent solution.

  1. For policy changed, send message to the 'system' topic and write to zookeeper, then 'system' topic consumer update policy cache in broker. So that just need to read the cache.
  2. For topic owner changed, load the namespace policy and topic policy into broker cache.

For 'system' topic design. I'm not sure the 'system' topic border.

  1. A 'system' topic in a cluster
  2. Namespace 'system' topic / Topic 'system' topic
  3. 'system' topic for each namespace / 'system' topic for each topic

I'm prone to 1 or 2, for 3 too much topic will be created.

@ivankelly Can you give me some idea more ? Thanks

@ivankelly
Copy link
Contributor

I would put it at the namespace level, so 2. It would be good to have a generic model for system topics, as currently it would require you wiring up something yourself. Maybe it would be better to put it at the managed ledger layer also, since if we put it at the pulsar level we would have a circular dependency of sorts.

@sijie
Copy link
Member

sijie commented Oct 23, 2018

@ivankelly I am a bit confused about using managed ledger. managed ledger doesn't have tailing facility, how do you propagate the policy changes using managed ledger?

@ivankelly
Copy link
Contributor

You shouldn't need to propagate. When you update the policy, you're hitting the broker that owns the topic.

@sijie
Copy link
Member

sijie commented Oct 23, 2018

@ivankelly

How does the broker get notified of updated to the policy for a topic? If we add a watcher, that's a watcher per topic.

You shouldn't need to propagate. When you update the policy, you're hitting the broker that owns the topic.

I am lost now. Isn't the existence of system topic to address the concern of zookeeper notifications? so we convert the zookeeper notifications, to consume from system topic. penghui's previous comment was using system topic, which is making sense, since pulsar already has producer and consumer. but in your last comment, you suggest using mledger to avoid circular dependency, then my question is "how is possible that the other brokers get notified with policy changes, because mledger doesn't have tailing facility".

@ivankelly
Copy link
Contributor

Isn't the existence of system topic to address the concern of zookeeper notifications

From what I can tell, system topics don't actually exist as an actual concept in pulsar, they're just normal topics. But as a conceptual concept, they don't exist just to replace notifications. They also exist as a place to store metadata which can scale.

Ok, so, to recap where the suggestion to use managed ledger came from...

The first suggestion was to put topic policies in zookeeper. This was discarded because of zookeeper scale issues.

So then it was suggested to use a system topic in pulsar. I'm questioning whether we should use a topic for policies on topics. What happens if the topic that contains the policy so has a topic policy? Maybe we need to prevent system topics from having topic level policies.

Otherwise, we put the "system topics" a lower layer and we wouldn't even have this problem. I suggested managed ledger, but dlog would also work here if we want notifications.

But stepping back, what I really want is that we define what system topics are and make them a reusable concept within the system.

@sijie
Copy link
Member

sijie commented Oct 23, 2018

But as a conceptual concept, they don't exist just to replace notifications. They also exist as a place to store metadata which can scale.

okay. if system topic is to store metadata, isn't it overlapped with the goal of building table service for metadata storage. if you just use a pure log to store metadata, we still have too much to build and most of the work is overlapped with the goal of table service.

so I think we should first target system topic as a changelog of what's happening within namespace, rather than making it to replace zookeeper to store metadata.

So then it was suggested to use a system topic in pulsar. I'm questioning whether we should use a topic for policies on topics. What happens if the topic that contains the policy so has a topic policy? Maybe we need to prevent system topics from having topic level policies.

good point. I see it now.

Otherwise, we put the "system topics" a lower layer and we wouldn't even have this problem. I suggested managed ledger, but dlog would also work here if we want notifications.

I think we should just consolidate managed ledger and dlog into one library and use it across the place. The storage layer already provide tailing as notifications, which pulsar should leverage for "system topics" here, maybe "readonly broker" in future, and even the sql segment reader optimization.

@ivankelly
Copy link
Contributor

I think we should just consolidate managed ledger and dlog into one library and use it across the place.

+1 to this.

@sijie sijie modified the milestones: 2.3.0, 2.4.0 Dec 27, 2018
@sijie
Copy link
Member

sijie commented Dec 27, 2018

@codelipenghui I moved this task to 2.4.0 since it seems that we need address system topic first.

@codelipenghui
Copy link
Contributor Author

@sijie @merlimat Are you already working on system topic now? I want to add dispatch rate limitation for a topic, it seems depends on system topic.

@codelipenghui codelipenghui modified the milestones: 2.4.0, 2.5.0 May 22, 2019
@sijie
Copy link
Member

sijie commented Aug 1, 2019

@codelipenghui @merlimat @ivankelly

After talking to a lot of users, users would like to have the ability to override the policy at namespace level. Since Pulsar doesn't support this feature, it forces the users to create many namespaces just for the ability to configure the policy for different topics.

I think we should just go ahead to implement the topic level policy without waiting for the system topic. If the concern is about zk access, we can think of reducing the zk access or having a flag to control whether to enable this feature or not and give warnings to people to not over-use this feature.

Thoughts?

huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this issue Aug 24, 2020
### Motivation
Master Issue: apache#2688
Add Topic level policy support for message TTL.

### Modifications

Support set/get/remove Message TTL on topic level.

### Verifying this change
This change added tests and can be verified as follows:

  - *Added Unit test to verify set/get/remove message TTL at Topic level work as expected when Topic level policy is enabled/disabled*
  - *Added test case in PersistentTopicE2ETest to verify Topic level message TTL is used when set and will fall back to namespace message TTL if Topic level message TTL is removed.*
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this issue Aug 24, 2020
Master Issue: apache#2688 

### Motivation
support topic level delayed delivery policy

### Modifications
Support set/get/remove delayed delivery policy on topic level.

### Verifying this change

Added Unit test to verify set/get/remove delayed delivery policy at Topic level work as expected when Topic level policy is enabled/disabled

- org.apache.pulsar.broker.admin.AdminApiDelayedDelivery#testEnableAndDisableTopicDelayedDelivery
- org.apache.pulsar.broker.admin.AdminApiDelayedDelivery#testEnableTopicDelayedDelivery
huangdx0726 referenced this issue in huangdx0726/pulsar Aug 24, 2020
lbenc135 pushed a commit to lbenc135/pulsar that referenced this issue Sep 5, 2020
### Motivation
Master Issue: apache#2688
Add Topic level policy support for message TTL.

### Modifications

Support set/get/remove Message TTL on topic level.

### Verifying this change
This change added tests and can be verified as follows:

  - *Added Unit test to verify set/get/remove message TTL at Topic level work as expected when Topic level policy is enabled/disabled*
  - *Added test case in PersistentTopicE2ETest to verify Topic level message TTL is used when set and will fall back to namespace message TTL if Topic level message TTL is removed.*
lbenc135 pushed a commit to lbenc135/pulsar that referenced this issue Sep 5, 2020
Master Issue: apache#2688 

### Motivation
support topic level delayed delivery policy

### Modifications
Support set/get/remove delayed delivery policy on topic level.

### Verifying this change

Added Unit test to verify set/get/remove delayed delivery policy at Topic level work as expected when Topic level policy is enabled/disabled

- org.apache.pulsar.broker.admin.AdminApiDelayedDelivery#testEnableAndDisableTopicDelayedDelivery
- org.apache.pulsar.broker.admin.AdminApiDelayedDelivery#testEnableTopicDelayedDelivery
lbenc135 referenced this issue in lbenc135/pulsar Sep 5, 2020
lbenc135 pushed a commit to lbenc135/pulsar that referenced this issue Sep 5, 2020
### Motivation
Master Issue: apache#2688
Add Topic level policy support for message TTL.

### Modifications

Support set/get/remove Message TTL on topic level.

### Verifying this change
This change added tests and can be verified as follows:

  - *Added Unit test to verify set/get/remove message TTL at Topic level work as expected when Topic level policy is enabled/disabled*
  - *Added test case in PersistentTopicE2ETest to verify Topic level message TTL is used when set and will fall back to namespace message TTL if Topic level message TTL is removed.*
lbenc135 pushed a commit to lbenc135/pulsar that referenced this issue Sep 5, 2020
Master Issue: apache#2688 

### Motivation
support topic level delayed delivery policy

### Modifications
Support set/get/remove delayed delivery policy on topic level.

### Verifying this change

Added Unit test to verify set/get/remove delayed delivery policy at Topic level work as expected when Topic level policy is enabled/disabled

- org.apache.pulsar.broker.admin.AdminApiDelayedDelivery#testEnableAndDisableTopicDelayedDelivery
- org.apache.pulsar.broker.admin.AdminApiDelayedDelivery#testEnableTopicDelayedDelivery
lbenc135 referenced this issue in lbenc135/pulsar Sep 5, 2020
lbenc135 pushed a commit to lbenc135/pulsar that referenced this issue Sep 5, 2020
### Motivation
Master Issue: apache#2688
Add Topic level policy support for message TTL.

### Modifications

Support set/get/remove Message TTL on topic level.

### Verifying this change
This change added tests and can be verified as follows:

  - *Added Unit test to verify set/get/remove message TTL at Topic level work as expected when Topic level policy is enabled/disabled*
  - *Added test case in PersistentTopicE2ETest to verify Topic level message TTL is used when set and will fall back to namespace message TTL if Topic level message TTL is removed.*
lbenc135 pushed a commit to lbenc135/pulsar that referenced this issue Sep 5, 2020
Master Issue: apache#2688 

### Motivation
support topic level delayed delivery policy

### Modifications
Support set/get/remove delayed delivery policy on topic level.

### Verifying this change

Added Unit test to verify set/get/remove delayed delivery policy at Topic level work as expected when Topic level policy is enabled/disabled

- org.apache.pulsar.broker.admin.AdminApiDelayedDelivery#testEnableAndDisableTopicDelayedDelivery
- org.apache.pulsar.broker.admin.AdminApiDelayedDelivery#testEnableTopicDelayedDelivery
lbenc135 referenced this issue in lbenc135/pulsar Sep 5, 2020
@codelipenghui
Copy link
Contributor Author

All topic policies are ready for 2.7.0. Thanks to all contributors work together to complete the great feature.

zymap pushed a commit that referenced this issue Mar 6, 2021
Fixes #8899

Update the description of the newly added topic-level policies in #2688.

This pr fixes #9161, the branch of which was deleted incorrectly.
mlyahmed pushed a commit to mlyahmed/pulsar that referenced this issue Mar 7, 2021
Fixes apache#8899

Update the description of the newly added topic-level policies in apache#2688.

This pr fixes apache#9161, the branch of which was deleted incorrectly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker area/cli type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

No branches or pull requests

3 participants