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

always truncate when SyncWrites is enabled #30

Closed
wants to merge 1 commit into from

Conversation

Stebalien
Copy link
Member

This will delete values that were in the process of being "put" when we crashed
but that's likely expected behavior anyways. Without this, ipfs on badger may
refuse to start after a crash.

part of ipfs/kubo#5275

@ghost ghost assigned Stebalien Aug 21, 2018
@ghost ghost added the status/in-progress In progress label Aug 21, 2018
@Stebalien Stebalien requested review from magik6k and schomatis August 21, 2018 04:54
This will delete values that were in the process of being "put" when we crashed
but that's likely expected behavior anyways. Without this, ipfs on badger may
refuse to start after a crash.
Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

We're assuming here (and it seems reasonable) that SyncWrites wasn't modified after the crash.

Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

I've been running few nodes with similar patch for quite some time now, it even survived few crashes

@Stebalien
Copy link
Member Author

We're assuming here (and it seems reasonable) that SyncWrites wasn't modified after the crash.

An alternative is to have a truncate config option and set it to true by default (like syncWrites). Actually, maybe we should do that instead. Gateways/caches will likely want to keep truncate enabled while disabling sync writes. Thoughts @schomatis @magik6k?

@schomatis
Copy link
Contributor

An alternative is to have a truncate config option

Yes, I agree, that was the intention behind ipfs/kubo#5275.

set it to true by default

I would prefer to set it to false by default, just to be conservative here, and document the possibility that a truncate might be needed after a crash and how the user can set it to true, but I don't really feel that strong about it, true is also ok.

@Stebalien
Copy link
Member Author

I would prefer to set it to false by default, just to be conservative here, and document the possibility that a truncate might be needed after a crash and how the user can set it to true, but I don't really feel that strong about it, true is also ok.

Given the semantics of our datastore, I'd prefer to set it to true (and document it). I'm pretty sure all (or almost all) filesystems/databases behave this way. Basically:

  1. If there is a bug in badger, we can lose data anyways.
  2. If there isn't a bug in badger, we haven't told the user that we've persisted the data so we're free to do anything we want.

Are there any cases I'm missing?

@schomatis
Copy link
Contributor

If there isn't a bug in badger, we haven't told the user that we've persisted the data so we're free to do anything we want.

If syncWrites is true by default that seems fair.

@Stebalien Stebalien closed this Aug 22, 2018
@ghost ghost removed the status/in-progress In progress label Aug 22, 2018
@Stebalien Stebalien deleted the fix/truncate-default branch August 22, 2018 02:37
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