Skip to content
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

PutAll ensures all futures finish #200

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

koba-e964
Copy link
Member

@koba-e964 koba-e964 commented Aug 26, 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)

Description of changes

frugalos_segment::client::PutAll が、与えられた future を全て確実に実行するようになる。

Behavior

frugalos_segment::client::PutAll が required_ok_count 個の future を成功裡に実行した後、残りの future を捨てる代わりに DefaultCpuTaskQueue に投げるようになる。

Purpose

Fixes #175.
PUT リクエストで保存すべきフラグメントが全部保存されない可能性があるという問題があり、今回の変更はそれを部分的に解決する。
この変更により、クライアントに応答を返した後も内部では Put リクエストを実行し続け、仮に失敗した時には PutAllMetricslost_fragments_total にカウントされる。
今後これに依存する形で以下のような PR を投げる予定。

  • DispersedClient::put が多重度の設定を受け取れるようにする
  • frugalos の PUT が多重度の設定を受け取れるようにする
  • frugalos の PUT の設定を DispersedClient::put まで伝播させる

Checklists

  • I have run cargo fmt --all.

@koba-e964 koba-e964 changed the title Feature/wait for puts completion PutAll ensures all futures finish Aug 26, 2019
@koba-e964 koba-e964 force-pushed the feature/wait-for-puts-completion branch from ca512bf to bc0b908 Compare August 26, 2019 11:48
@koba-e964 koba-e964 marked this pull request as ready for review August 26, 2019 11:57
Err(_) => unreachable!(),
}
thread::sleep(Duration::from_millis(1));
};
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Needs comments to justify waiting time of 1ms, or possibly a better alternative.

Copy link
Member Author

Choose a reason for hiding this comment

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

src/daemon.rs の spawner をここまで持ってきて、spawner.spawn で Future を実行する方法もある。

Copy link
Member Author

Choose a reason for hiding this comment

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

待つ時間は長くてもそこまで問題にはならなそうで、spawner を持ってくるのは変更が大きそうという理由から、待つ時間を環境変数で設定できるようにします (デフォルトを 100ms くらいにします)
根拠:

  • 待つ時間は長くても問題にならない: このコードだと PUT アクセス1個につき1個のスレッドが立つ。このスレッドは PUT が全部終わるのを待って終了するため、PUT アクセスにかかる時間程度までなら待ち時間によらずかかる時間はほぼ一定であるため。
  • spawner を持ってくるのは変更が大きそう: Bucket -> Client -> StorageClient -> DispersedClient -> PutAll と順に変更する必要があるため。
  • 100ms: 根拠に乏しいが PUT より充分短い時間であるため。

Copy link
Member Author

Choose a reason for hiding this comment

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

この実装だと、tasque_task_duration_seconds というメトリクスの quantile クエリがほぼ残りの PUT 由来となります。問題かどうかはよくわかりません。

@koba-e964 koba-e964 changed the base branch from refactor/segment_storage to develop August 28, 2019 08:00
@koba-e964 koba-e964 force-pushed the feature/wait-for-puts-completion branch from 840ca5e to 52f72b9 Compare December 12, 2019 10:16
@shinnya shinnya added this to the v0.17.0 milestone Dec 17, 2019
@koba-e964 koba-e964 modified the milestones: v0.17.0, v0.18.0 Dec 18, 2019
@shinnya shinnya removed this from the v0.18.0 milestone Jan 21, 2020
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.

PutAll should wait for the completion of all put operations
2 participants