[fix](feut) fix MetaServiceProxyTest - #66219
Open
mymeiyi wants to merge 1 commit into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes MetaServiceProxyTest flakiness by making the rate-limit exhaustion deterministic and updating the RPC invocation expectation accordingly.
Changes:
- Adjusts rate-limit configuration to use a longer refresh window.
- Consumes the entire quota via a single weighted
getVisibleVersionAsynccall. - Updates Mockito verification to expect one RPC call (the quota-consuming call) instead of none.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
### What problem does this PR solve? Issue Number: None Related PR: apache#66219 Problem Summary: MetaServiceProxyTest exhausted rate-limit permits through repeated calls with a one-second refresh period, so scheduler delays could replenish permits and make the assertion flaky. Consume the configured limit for a longer burst window with one weighted request, derive that weight explicitly from the QPS and burst settings, and return immediately completed mocked futures so async callbacks do not remain pending. ### Release note None ### Check List (For Author) - Test: Unit Test - `./run-fe-ut.sh --run org.apache.doris.cloud.rpc.MetaServiceProxyTest` (11 tests passed on the development machine) - FE Checkstyle passed with 0 violations - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
fe/fe-core/src/test/java/org/apache/doris/cloud/rpc/MetaServiceProxyTest.java:324
limitForPeriodscales withCPU_CORESand a hardcoded 60s burst window, which can make this unit test generate very large requests on high-core CI agents (and potentially slow down / increase memory usage). Consider capping the computed weight to a small upper bound for tests (e.g.,min(computed, 100)), and/or using a smallerrateLimitBurstSecondswhile keeping the test deterministic. Also, computing this withintrisks overflow on unusually large environments—preferlongfor the multiplication and only cast down if needed.
int qpsPerCore = 1;
int rateLimitBurstSeconds = 60;
int limitForPeriod = qpsPerCore * CPU_CORES * rateLimitBurstSeconds;
enableRateLimit(qpsPerCore, "", rateLimitBurstSeconds, 0);
fe/fe-core/src/test/java/org/apache/doris/cloud/rpc/MetaServiceProxyTest.java:320
- This comment indicates the test is working around timing/scheduler behavior by using a long burst window, which can keep the test brittle and harder to reason about. A more maintainable approach is to make the rate-limit refill/refresh deterministic in tests (e.g., inject a controllable clock/ticker or explicitly disable/avoid background refresh) so assertions don’t depend on scheduler progress.
// Consume limitForPeriod = qpsPerCore * CPU_CORES * burstSeconds in one weighted request.
// A long burst window also prevents a refresh before the assertion if the scheduler stalls.
Contributor
Author
|
run buildall |
Contributor
|
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29275 ms |
Contributor
TPC-DS: Total hot run time: 176528 ms |
Contributor
ClickBench: Total hot run time: 24.96 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review opinion: approve. I found no blocking or new non-duplicate issues at head 25e20bd4b91ea5f92b6d35d7e8b4290ea8cc544d.
Critical checkpoints:
- Goal and proof: the revised partition-version test derives the weighted request from the same
qpsPerCore * CPU_CORES * burstSecondsformula as the production limiter, consumes the full period in one request, and verifies that the next request is rejected before a second RPC. The table-version path retains equivalent weighted-permit coverage. - Scope and clarity: this is a focused test-only patch. It changes only the two affected async rate-limit tests and adds the completed-future import.
- Concurrency and lifecycle: no new threads or locks are introduced.
Futures.immediateFutureruns the existing direct-executor success callback without leaving pending callback state or entering the client-shutdown path; the separate controlled failure test still covers delayed failure and shutdown. Static config and limiter state are restored/reset by the fixture, and FE tests use non-reused forks for singleton isolation. - Conditions and error handling: the rate-limit exception remains asserted, the Mockito count proves the rejected call does not reach the client, and no status/exception is silently discarded.
- Configuration, compatibility, persistence, data writes, FE-BE propagation, and observability: no production behavior or configuration is changed, so these checkpoints are not applicable.
- Performance and resources: the test builds one request proportional to
60 * availableProcessors()and performs one mocked RPC; this is bounded test-only work and does not affect a production hot path. - Tests and results: the patch updates targeted JUnit coverage; no regression
.outfiles are involved. The PR reports 11MetaServiceProxyTesttests and Checkstyle passing. I did not run builds or tests in this review-only runner. - Existing review context: the current head addresses the earlier naming, capacity derivation, and never-completed-future concerns. The remaining theoretical refresh-window/high-core observations are already represented in the existing review context and do not support a distinct inline finding.
- User focus: no additional review focus was provided; the whole PR was reviewed.
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.