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

削除についてはjournal領域変更が先、データ領域変更が後 #19

Merged
merged 3 commits into from Jan 21, 2019

Conversation

yuezato
Copy link
Member

@yuezato yuezato commented Jan 21, 2019

No description provided.

sile
sile previously approved these changes Jan 21, 2019
Copy link
Member

@sile sile left a comment

Choose a reason for hiding this comment

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

LGTM

@yuezato yuezato merged commit b908df6 into master Jan 21, 2019
@shinnya
Copy link
Contributor

shinnya commented Jan 22, 2019

@yuezato Please explain why this change was required.

@shinnya
Copy link
Contributor

shinnya commented Jan 22, 2019

I've found that the reason why this change was required was discussed on gitter.

@yuezato
Copy link
Member Author

yuezato commented Jan 22, 2019

後でGitterを遡るのは辛いので念のため書いておきます。

最大の理由は、delete_if_existsが行っているようにジャーナル領域への削除通知を先に行い、その上でデータ領域の削除を行うという流れにしたかったから、です:

cannyls/src/storage/mod.rs

Lines 371 to 386 in b908df6

fn delete_if_exists(&mut self, lump_id: &LumpId, do_record: bool) -> Result<bool> {
if let Some(portion) = self.lump_index.remove(lump_id) {
self.metrics.delete_lumps.increment();
if do_record {
track!(self
.journal_region
.records_delete(&mut self.lump_index, lump_id,))?;
}
if let Portion::Data(portion) = portion {
self.data_region.delete(portion);
}
Ok(true)
} else {
Ok(false)
}
}

ジャーナル領域への削除通知を後回しにしてしまうと、ジャーナル領域変更直前にcannylsがクラッシュした場合、復旧後にはcannylsのindex情報復元の都合上、削除前の状態に巻き戻ってしまいます。

何らかの性質を満たすためにdelete_rangeでジャーナル領域への変更を後ろに回しているなら良かったのですが、そういうわけではなかったので、この段階で自然な計算の流れに戻しました。

@shinnya
Copy link
Contributor

shinnya commented Jan 22, 2019

ありがとうございます

@shinnya shinnya deleted the journal_saki_delete_ato branch March 28, 2019 04:29
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.

None yet

3 participants