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

[Issue 11581][broker] feat:pass the executor to RateLimiter in ResourceGroupPublishLimiter #11582

Merged
merged 5 commits into from Aug 21, 2021

Conversation

leizhiyuan
Copy link
Contributor

background

Fixes #11581

Motivation

we need to pass the executor to RateLimiter in ResourceGroupPublishLimiter

Modifications

pass the executor to RateLimiter in ResourceGroupPublishLimiter

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

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:(no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options:(no)
  • Anything that affects deployment: (no)

Documentation

For contributor

For this PR, do we need to update docs?

no, there is no change for users.

@Anonymitaet Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Aug 9, 2021
@Anonymitaet
Copy link
Member

Thanks for providing doc-related info!

.permits(publishMaxMessageRate)
.rateTime(1L)
.timeUnit(TimeUnit.SECONDS)
.isDispatchOrPrecisePublishRateLimiter(false)
Copy link
Member

@lhotari lhotari Aug 9, 2021

Choose a reason for hiding this comment

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

the default value is false, so there's no need to set isDispatchOrPrecisePublishRateLimiter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@leizhiyuan
Copy link
Contributor Author

image

the failed tase case runs ok in local machine

@lhotari
Copy link
Member

lhotari commented Aug 9, 2021

the failed tase case runs ok in local machine

@leizhiyuan There are quite a few flaky tests. I assume you faced #11096 .

When you suspect a flaky test, please search in the GitHub issue with the short name of the test class for possible reported flaky tests.

It is possible to retry a failed CI build by adding a comment "/pulsarbot rerun-failure-checks" to the PR.

  • Before retrying a failed build, please check the failure in the logs. Check the issue tracker whether the test failure has been reported as a flaky test. If not, please report it.
  • If it has been reported, please add a comment about facing a flaky issue in a specific build (provide link to log line in GitHub Actions log).

Reporting a new Flaky issue can be done by choosing "Flaky test" on the new
issue page https://github.com/apache/pulsar/issues/new/choose .
Searching flaky issues:
https://github.com/apache/pulsar/issues?q=is%3Aissue+is%3Aopen+flaky+sort%3Aupdated-desc
(enter "flaky" or the test method name as the search query)

# Conflicts:
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupPublishLimiter.java
@bharanic-dev
Copy link
Contributor

@leizhiyuan Thank you for taking care of this. Passing the Executor was one of the tasks in my TODO list (which I why I left the comment in the code). Glad you were able to get to it before I could.

Just to clarify though, I don't think this addresses any flaky tests. The flaky tests reported in:
#11096
#11601

are because of a code issue, where the zookeeper watch events were getting fixed. The race condition was reported in #11157 and fixed in #11198

@bharanic-dev
Copy link
Contributor

LGTM

@sijie sijie added this to the 2.9.0 milestone Aug 21, 2021
@sijie sijie merged commit f7957a5 into apache:master Aug 21, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
…ceGroupPublishLimiter (apache#11582)

### background

Fixes apache#11581 

### Motivation

we need to pass the executor to RateLimiter in ResourceGroupPublishLimiter

### Modifications

 pass the executor to RateLimiter in ResourceGroupPublishLimiter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pass the executor to RateLimiter in ResourceGroupPublishLimiter
7 participants