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

db: call Sync after deletion threshold #3122

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

darshanime
Copy link

Add periodic calls to (objstorage.Provider).Sync

Closes: #3025

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jbowens jbowens requested review from RaduBerinde and a team December 5, 2023 15:46
Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @darshanime)


cleaner.go line 176 at r1 (raw file):

func (cm *cleanupManager) deleteObsoleteObjectAndSyncIfRequired(jobID int, fileNum base.DiskFileNum, fileSize uint64) {
	var deleteThresholdForSync uint64 = 64 * 1024 * 1024 // 64 MB

[nit] this should be a constant around the top of the file (next to jobsQueueDepth)


cleaner.go line 188 at r1 (raw file):

	}

	if err := cm.objProvider.Sync(); err == nil {

We should log the error here. I think we should still reset deletedFileSizePendingSync even in the error case, it's unlikely that retrying after every object will help.


cleaner.go line 265 at r1 (raw file):

}

func (cm *cleanupManager) deleteObsoleteObject(

This needs an explanation of the return value. Though I don't think it's worth it, the case of the object not existing is unlikely to happen in practice


testdata/cleaner line 190 at r1 (raw file):

close: db1/000123.sst

create-bogus-file db1/000456.sst 100000000

Add a comment explaining that we're setting a large file size to verify that the cleaner issues a sync.

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.

db: periodically sync the data directory during file deletions
3 participants