8000 Add storage-head feature by brly · Pull Request #216 · frugalos/frugalos · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 4 commits into from
Oct 4, 2019
Merged

Conversation

brly
Copy link
Member
@brly brly commented Sep 30, 2019

Types of changes

Please check one of the following:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • New release (merge to both master and develop!)

Description of changes

Behavior

オブジェクトへの HEAD リクエスト種別に storage (cannyls) にも存在確認を問い合わせられるようにする。
frugalos/libfrugalos#24 の PR の変更も必要。

Purpose

既存の HEAD リクエストでは mds までしか存在確認が行われなく、mds と cannyls で不整合が起きていた場合には既存の HEAD では判定出来ないため。

Checklists

  • Run cargo fmt --all.
  • Run cargo clippy --all --all-targets.

@brly brly force-pushed the feature/head-storage branch from e5a8bf2 to 22f6f51 Compare October 1, 2019 08:03
@brly brly changed the title [WIP] Add storage-head feature Add storage-head feature Oct 1, 2019
Copy link
Contributor
@koba-e964 koba-e964 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits です

@brly brly force-pushed the feature/head-storage branch from 22f6f51 to 1b3039c Compare October 2, 2019 14:02
@@ -69,6 +69,9 @@ impl ReplicatedClient {
};
Box::new(future)
}
pub fn head(self, _version: ObjectVersion, _deadline: Deadline) -> BoxFuture<()> {
8000 Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO コメントとかをつけていただけると助かります

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

旨を記述しておきます

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

未実装の場合 unreachable!() を実装にしている既存コードがあります。が、これが普通なのかまでは分かっていません…。

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

なるほど。
今回は未実装なんですけど、コードとしては到達してしまうのでマクロは置かないでおきますかね。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここは普通に到達する (Replicated バケツのオブジェクトに HEAD) ので、unreachable! にはしない方がいいのではという気がします。warn! とかはしてもいいかもという気がします。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unreachable! を使ってるコードは本当に unreachable だったかもしれません。

Copy link
Contributor

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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

デフォルトは既存の get_timeout に合わせていますが、短くても良い気もしています。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0