[Remote Compaction] Introduce kAborted Status#13438
Conversation
There was a problem hiding this comment.
Sorry - can we have a little more context on this though the code looks fine in general?
Are we planning to do something different between kAborted and kFailure in rocksdb and/or remote compaction side? For the client (i.e, rocksdb that is looking at the result from Schedule()/Wait()..), the error kAborted is not indicating anything different from kFailure in practice right now IMO. Maybe some comments on the API that can return kAborted is needed.
- Is this usually an error allowing re-try? For retry we have Status::kTryAgain().
If compaction job needs to be aborted inside Schedule() or Wait() today (e.g. Primary host is shutting down)
Shouldn't it be the "secondary host" is shutting down if the status is returned from Schedule() or Wait()?? Ah i see this is for the case where primary host shutting down will also abort the remote compaction
|
The context requires some Meta internal implementation info. Let me sync with you offline. |
hx235
left a comment
hx235
left a comment
There was a problem hiding this comment.
After talking to the author it's now clearer to me why kAborted is needed though from RocksDB point of view currently we don't differentiate kAborted from kFailure
jaykorean
force-pushed
the
remote_compaction_aborted_status
branch
from
f4cfdbf to
b64cf5b
Compare
|
@jaykorean has updated the pull request. You must reimport the pull request before landing. |
|
@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@jaykorean merged this pull request in 68b2d94. |
Summary: If compaction job needs to be aborted inside `Schedule()` or `Wait()` today (e.g. Primary host is shutting down), the only two options are the following - Handle it as failure by returning `CompactionServiceJobStatus::kFailure` - Return `CompactionServiceJobStatus::kUseLocal` and let the compaction move on locally and eventually succeed or fail depending on the timing In this PR, we are introducing a new status, `CompactionServiceJobStatus::kAborted`, so that the implementation of `Schedule()` and `Wait()` can return it. Just like how `CompactionServiceJobStatus::kFailure` is handled, compaction will not move on and fail, but the status will be returned as `Status::Aborted()` instead of `Status::Incomplete()` Pull Request resolved: #13438 Test Plan: Unit Test added ``` ./compaction_service_test --gtest_filter="*CompactionServiceTest.AbortedWhileWait*" ``` Reviewed By: anand1976, hx235 Differential Revision: D70655355 Pulled By: jaykorean fbshipit-source-id: 22614ce9c7455cda649b15465625edc93978fe11
Summary: If compaction job needs to be aborted inside `Schedule()` or `Wait()` today (e.g. Primary host is shutting down), the only two options are the following - Handle it as failure by returning `CompactionServiceJobStatus::kFailure` - Return `CompactionServiceJobStatus::kUseLocal` and let the compaction move on locally and eventually succeed or fail depending on the timing In this PR, we are introducing a new status, `CompactionServiceJobStatus::kAborted`, so that the implementation of `Schedule()` and `Wait()` can return it. Just like how `CompactionServiceJobStatus::kFailure` is handled, compaction will not move on and fail, but the status will be returned as `Status::Aborted()` instead of `Status::Incomplete()` Pull Request resolved: facebook#13438 Test Plan: Unit Test added ``` ./compaction_service_test --gtest_filter="*CompactionServiceTest.AbortedWhileWait*" ``` Reviewed By: anand1976, hx235 Differential Revision: D70655355 Pulled By: jaykorean fbshipit-source-id: 22614ce9c7455cda649b15465625edc93978fe11
4 participants
Summary
If compaction job needs to be aborted inside
Schedule()orWait()today (e.g. Primary host is shutting down), the only two options are the followingCompactionServiceJobStatus::kFailureCompactionServiceJobStatus::kUseLocaland let the compaction move on locally and eventually succeed or fail depending on the timingIn this PR, we are introducing a new status,
CompactionServiceJobStatus::kAborted, so that the implementation ofSchedule()andWait()can return it. Just like howCompactionServiceJobStatus::kFailureis handled, compaction will not move on and fail, but the status will be returned asStatus::Aborted()instead ofStatus::Incomplete()Test Plan
Unit Test added