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

support message dispatch rate on topic level #7863

Merged
merged 5 commits into from
Aug 28, 2020

Conversation

hangc0276
Copy link
Contributor

Motivation

Support message dispatch rate on topic level.
Based on the system topic function.

Modifications

Support set message dispatch rate on topic level.
Support get message dispatch rate on topic level.
Support remove message dispatch rate on topic level.

Verifying this change

This change added tests and can be verified as follows:

  • test set topic message dispatch rate
  • test remove topic message dispatch rate
  • test get topic message dispatch rate
  • test disabled topic message dispatch rate

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes)
  • Anything that affects deployment: (no)

Documentation

Does this pull request introduce a new feature? (yes)
If yes, how is the feature documented? (docs / JavaDocs)

@hangc0276
Copy link
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui codelipenghui added this to the 2.7.0 milestone Aug 21, 2020
@codelipenghui codelipenghui added component/topic-policy doc-required Your PR changes impact docs and you will update later. labels Aug 21, 2020
@codelipenghui
Copy link
Contributor

@MarvinCai @jianyun8023 Please help review this PR, thanks.

@hangc0276
Copy link
Contributor Author

/pulsarbot run-failure-checks

Copy link
Contributor

@jianyun8023 jianyun8023 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave a small comment.

@@ -3056,4 +3057,53 @@ protected void internalGetLastMessageId(AsyncResponse asyncResponse, boolean aut
});

}

protected void internalGetDispatchRate(AsyncResponse asyncResponse) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is recommended to return CompletableFuture instead of passing in the parameter AsyncResponse

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the implement interface, please take a look again. Thanks


@Override
public void unregisterListener(TopicName topicName, TopicPolicyListener<TopicPolicies> listener) {
listeners.computeIfAbsent(topicName, k -> Lists.newCopyOnWriteArrayList());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this remove all listeners for a given topic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should remove the specific listener, and i have updated the code, please take a look again. Thanks

@hangc0276
Copy link
Contributor Author

/pulsarbot run-failure-checks

3 similar comments
@sijie
Copy link
Member

sijie commented Aug 24, 2020

/pulsarbot run-failure-checks

@hangc0276
Copy link
Contributor Author

/pulsarbot run-failure-checks

@hangc0276
Copy link
Contributor Author

/pulsarbot run-failure-checks

@hangc0276
Copy link
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui codelipenghui merged commit 395037e into apache:master Aug 28, 2020
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
### Motivation
Support message dispatch rate on topic level.
Based on the system topic function.

### Modifications
Support set message dispatch rate on topic level.
Support get message dispatch rate on topic level.
Support remove message dispatch rate on topic level.
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
### Motivation
Support message dispatch rate on topic level.
Based on the system topic function.

### Modifications
Support set message dispatch rate on topic level.
Support get message dispatch rate on topic level.
Support remove message dispatch rate on topic level.
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
### Motivation
Support message dispatch rate on topic level.
Based on the system topic function.

### Modifications
Support set message dispatch rate on topic level.
Support get message dispatch rate on topic level.
Support remove message dispatch rate on topic level.
jiazhai pushed a commit that referenced this pull request Sep 9, 2020
Fixes #7863

### Motivation
1) Topic level policy will be overwritten by namespace level
2) After removing the topic level policy, `DispatchLimiter` does not take effect,  the namespace level policy should be used

### Modifications
1)If there is a topic-level policy, it will not be overwritten by the namespace-level policy when the policy is updated
2)When removing topic-level policy, namespace-level policy will be used. If the namespace-level policy does not exist, the broker level will be used

### Verifying this change
unit test:
TopicPoliciesTest#testPolicyOverwrittenByNamespaceLevel
@codelipenghui codelipenghui mentioned this pull request Sep 9, 2020
14 tasks
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Oct 3, 2020
Fixes apache#7863

### Motivation
1) Topic level policy will be overwritten by namespace level
2) After removing the topic level policy, `DispatchLimiter` does not take effect,  the namespace level policy should be used

### Modifications
1)If there is a topic-level policy, it will not be overwritten by the namespace-level policy when the policy is updated
2)When removing topic-level policy, namespace-level policy will be used. If the namespace-level policy does not exist, the broker level will be used

### Verifying this change
unit test:
TopicPoliciesTest#testPolicyOverwrittenByNamespaceLevel
codelipenghui pushed a commit that referenced this pull request Feb 5, 2021
### Motivation

Maybe CI jobs are failing with OOM in the brokers unit tests. The Surefire worker is configured with 4 processes, each with xmx of 1G. 

The problem was introduced in #7863 where a static map of listeners was added to an interface. That makes that map to contain all the `PulsarService` instances created during the tests execution and keeping references to everything else.

The map should instead be scoped to the specific instance.
codelipenghui pushed a commit that referenced this pull request Feb 5, 2021
Maybe CI jobs are failing with OOM in the brokers unit tests. The Surefire worker is configured with 4 processes, each with xmx of 1G.

The problem was introduced in #7863 where a static map of listeners was added to an interface. That makes that map to contain all the `PulsarService` instances created during the tests execution and keeping references to everything else.

The map should instead be scoped to the specific instance.

(cherry picked from commit 31ee454)
merlimat added a commit to merlimat/pulsar that referenced this pull request Apr 6, 2021
…9486)

### Motivation

Maybe CI jobs are failing with OOM in the brokers unit tests. The Surefire worker is configured with 4 processes, each with xmx of 1G. 

The problem was introduced in apache#7863 where a static map of listeners was added to an interface. That makes that map to contain all the `PulsarService` instances created during the tests execution and keeping references to everything else.

The map should instead be scoped to the specific instance.
@Anonymitaet Anonymitaet added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-required Your PR changes impact docs and you will update later. labels Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants