Add arbitration lock time out to shared arbitrator #11376
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
tanjialiang
force-pushed
the
arb_timeout
branch
from
60f47f2 to
318d594
Compare
tanjialiang
force-pushed
the
arb_timeout
branch
2 times, most recently
from
3c0f916 to
c71904a
Compare
| @@ -1221,12 +1203,12 @@ void SharedArbitrator::freeCapacity(uint64_t bytes) { | |||
| if (FOLLY_UNLIKELY(bytes == 0)) { | |||
| return; | |||
| } | |||
| std::vector<ContinuePromise> resumes; | |||
| std::vector<ContinuePromise> globalArbitrationWaitResumes; | |||
There was a problem hiding this comment.
Can we just use short names which is clear in the context? Thanks!
There was a problem hiding this comment.
In freeCapacity() method scope there isn't any context mentioning the resume is for global arbitration. So I changed the name to indicate. This way I don't need to refer to other locations of the file to figure out this resume is for global arbitration anymore.
| @@ -3935,5 +3933,90 @@ TEST_F(MockSharedArbitrationTest, concurrentArbitrationWithTransientRoots) { | |||
| } | |||
| controlThread.join(); | |||
| } | |||
| } // namespace | |||
|
|
|||
| TEST_F(MockSharedArbitrationTest, arbitrationOperationTimedLock) { | |||
There was a problem hiding this comment.
This test should be in ArbitrationParticipantTest? Also cover the two APIs: abort and reclaim?
In MockSharedArbirrationTest, let's have two test cases with one for reclaim lock timeout and one for abort lock time out? Thanks!
tanjialiang
force-pushed
the
arb_timeout
branch
4 times, most recently
from
2d0f96a to
81961c7
Compare
|
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
tanjialiang
force-pushed
the
arb_timeout
branch
6 times, most recently
from
ad7d8ed to
dea0450
Compare
|
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
tanjialiang
force-pushed
the
arb_timeout
branch
from
dea0450 to
fc3ba9d
Compare
|
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
tanjialiang
force-pushed
the
arb_timeout
branch
from
fc3ba9d to
3d89b31
Compare
|
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
tanjialiang
force-pushed
the
arb_timeout
branch
from
3d89b31 to
9afa8c5
Compare
|
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
|
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
tanjialiang
force-pushed
the
arb_timeout
branch
from
9afa8c5 to
b2d3ef5
Compare
|
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…#11376) Summary: Current arbitrator lock does not enforce timeout, which might lead to arbitration operation's timeout not strictly enforced if waiting for too long for a lock. It can also prevent deadlock situation from completely hanging the system. Reviewed By: xiaoxmeng Differential Revision: D65151919 Pulled By: tanjialiang
tanjialiang
force-pushed
the
arb_timeout
branch
from
b2d3ef5 to
4a5a73d
Compare
|
This pull request was exported from Phabricator. Differential Revision: D65151919 |
|
@tanjialiang merged this pull request in 3e93c07. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Current arbitrator lock does not enforce timeout, which might lead to arbitration operation's timeout not strictly enforced if waiting for too long for a lock. It can also prevent deadlock situation from completely hanging the system.