-
Notifications
You must be signed in to change notification settings - Fork 25
Add storage-head feature #216
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
Conversation
e5a8bf2
to
22f6f51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits です
22f6f51
to
1b3039c
Compare
@@ -69,6 +69,9 @@ impl ReplicatedClient { | |||
}; | |||
Box::new(future) | |||
} | |||
pub fn head(self, _version: ObjectVersion, _deadline: Deadline) -> BoxFuture<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO コメントとかをつけていただけると助かります
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
旨を記述しておきます
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
未実装の場合 unreachable!()
を実装にしている既存コードがあります。が、これが普通なのかまでは分かっていません…。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほど。
今回は未実装なんですけど、コードとしては到達してしまうのでマクロは置かないでおきますかね。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここは普通に到達する (Replicated バケツのオブジェクトに HEAD) ので、unreachable!
にはしない方がいいのではという気がします。warn!
とかはしてもいいかもという気がします。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unreachable!
を使ってるコードは本当に unreachable だったかもしれません。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
現状で LGTM です
@@ -197,6 +206,10 @@ fn default_dispersed_client_get_timeout() -> Duration { | |||
Duration::from_secs(2) | |||
} | |||
|
|||
fn default_dispersed_client_head_timeout() -> Duration { | |||
Duration::from_secs(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
デフォルトは既存の get_timeout に合わせていますが、短くても良い気もしています。
Types of changes
Please check one of the following:
master
anddevelop
!)Description of changes
Behavior
オブジェクトへの HEAD リクエスト種別に storage (cannyls) にも存在確認を問い合わせられるようにする。
frugalos/libfrugalos#24 の PR の変更も必要。
Purpose
既存の HEAD リクエストでは mds までしか存在確認が行われなく、mds と cannyls で不整合が起きていた場合には既存の HEAD では判定出来ないため。
Checklists
cargo fmt --all
.cargo clippy --all --all-targets
.