You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2022/09/24 09:51:05 UTC
[GitHub] [zookeeper] kezhuw opened a new pull request, #1887: ZOOKEEPER-4327: Fix flaky RequestThrottlerTest
kezhuw opened a new pull request, #1887:
URL: https://github.com/apache/zookeeper/pull/1887
This PR tries to fix several test failures in `RequestThrottlerTest`.
First, `RequestThrottlerTest#testDropStaleRequests`.
Place `Thread.sleep(200)` after `submittedRequests.take()` in `RequestThrottler#run` will fail two assertions:
1. `assertEquals(2L, (long) metrics.get("prep_processor_request_queued"))`
2. `assertEquals(1L, (long) metrics.get("request_throttle_wait_count"))`
This happens due to `setStale` chould happen before throttle handling.
This commit solves this by introducing an interception point `RequestThrottler.throttleSleep` to build happen-before relations:
1. `throttling.countDown` happens before `setStale`, this ensures that unthrottled request are processed as usual.
2. `setStale` happens before `throttled.await`, this defends `RequestThrottler.throttleSleep` against spurious wakeup.
Second, `RequestThrottlerTest#testRequestThrottler`.
* `RequestThrottlerTest.testRequestThrottler:197 expected: <2> but was: <1>`
`ZooKeeperServer#submitRequest` and `PrepRequestProcessor#processRequest` run in different threads, thus there is no guarantee on metric `prep_processor_request_queued` after `submitted.await(5, TimeUnit.SECONDS)`. Place `Thread.sleep(200)` before `zks.submitRequestNow(request)` in `RequestThrottler#run` will incur this failure.
* `RequestThrottlerTest.testRequestThrottler:206 expected: <5> but was: <4>`
`entered.await(STALL_TIME, TimeUnit.MILLISECONDS)` could return `false` due to almost same timeout as `RequestThrottler#throttleSleep`. Place `Thread.sleep(500)` around `throttleSleep` will increase failure possibility.
Third, `RequestThrottlerTest#testGlobalOutstandingRequestThrottlingWithRequestThrottlerDisabled`.
* `RequestThrottlerTest.testGlobalOutstandingRequestThrottlingWithRequestThrottlerDisabled:340 expected: <3> but was: <4>`
`ZooKeeperServer#shouldThrottle` depends on consistent sum of `getInflight` and `getInProcess`. But it is no true. Place `Thread.sleep(200)` before `zks.submitRequestNow(request)` in `RequestThrottler#run` could reproduce this.
Sees also https://github.com/apache/zookeeper/pull/1739, https://github.com/apache/zookeeper/pull/1821.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on pull request #1887: ZOOKEEPER-4327: Fix flaky RequestThrottler#testDropStaleRequests
Posted by GitBox <gi...@apache.org>.
kezhuw commented on PR #1887:
URL: https://github.com/apache/zookeeper/pull/1887#issuecomment-1135997737
@maoling @eolivelli @symat PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling closed pull request #1887: ZOOKEEPER-4327: Fix flaky RequestThrottlerTest
Posted by GitBox <gi...@apache.org>.
maoling closed pull request #1887: ZOOKEEPER-4327: Fix flaky RequestThrottlerTest
URL: https://github.com/apache/zookeeper/pull/1887
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on pull request #1887: ZOOKEEPER-4327: Fix flaky RequestThrottlerTest
Posted by GitBox <gi...@apache.org>.
kezhuw commented on PR #1887:
URL: https://github.com/apache/zookeeper/pull/1887#issuecomment-1168690058
I overlooked test failures unrelated to `testLargeRequestThrottling` in #1821. I try to fix all remaining test failures in `RequestThrottlerTest` in this pr. All of these failures could be reproduced by placing `Thread.sleep(200)` at some places.
@eolivelli @maoling @symat Could you please take a look ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on pull request #1887: ZOOKEEPER-4327: Fix flaky RequestThrottler#testDropStaleRequests
Posted by GitBox <gi...@apache.org>.
kezhuw commented on PR #1887:
URL: https://github.com/apache/zookeeper/pull/1887#issuecomment-1135987024
This fixes failed ci after #1821 merged(https://github.com/apache/zookeeper/commit/c05ca0ee0deedd50d0ef32e3e01ad7f691d27009) https://github.com/apache/zookeeper/runs/6568872986?check_suite_focus=true
P.S. Place `Thread.sleep(200)` after `submittedRequests.take()` in
`RequestThrottler#run` will fail these assertions also before #1821, but for different reason due to changes in #1821.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on pull request #1887: ZOOKEEPER-4327: Fix flaky RequestThrottlerTest
Posted by GitBox <gi...@apache.org>.
kezhuw commented on PR #1887:
URL: https://github.com/apache/zookeeper/pull/1887#issuecomment-1272737030
@maoling @symat Thank you for merging and reviewing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1887: ZOOKEEPER-4327: Fix flaky RequestThrottlerTest
Posted by GitBox <gi...@apache.org>.
maoling commented on PR #1887:
URL: https://github.com/apache/zookeeper/pull/1887#issuecomment-1257147184
- @kezhuw Great work. Sorry for our late.Thanks for your contribution.
- Commit to the following branches:
[master ](https://github.com/apache/zookeeper/tree/master)
[branch-3.8](https://github.com/apache/zookeeper/tree/branch-3.8)
[branch-3.7](https://github.com/apache/zookeeper/tree/branch-3.7)
, except [branch-3.6](https://github.com/apache/zookeeper/tree/branch-3.6)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] asfgit closed pull request #1887: ZOOKEEPER-4327: Fix flaky RequestThrottlerTest
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1887: ZOOKEEPER-4327: Fix flaky RequestThrottlerTest
URL: https://github.com/apache/zookeeper/pull/1887
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org