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

[Badger Batch] Chunk Data Packs and ConsumeProgress #6391

Closed

Conversation

zhangchiqing
Copy link
Member

@zhangchiqing zhangchiqing commented Aug 23, 2024

This PR refactored the storage module of chunk data packs and consumer progress to use badger batch updates.

There are many file changes, primarily due to a refactor that implements the factory pattern, which addresses a possible API misuse. I've provided further explanation in the comments below.

if err != nil {
return 0, fmt.Errorf("failed to retrieve processed index: %w", err)
}
return processed, nil
}

// InitProcessedIndex insert the default processed index to the storage layer, can only be done once.
// initialize for the second time will return storage.ErrAlreadyExists
Copy link
Member Author

@zhangchiqing zhangchiqing Aug 23, 2024

Choose a reason for hiding this comment

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

With batch updates, it's not easy to prevent dirty reads in this call when checking the case that might return ErrAlreadyExists. We would need to introduce a lock.

Instead of using lock, I decided to move this check to a Factory struct, since it's easy for the owner of the Factory to ensure initializing in a single thread, and then the returned consumer progress could be concurrent-safe. This eliminates the uninitialized state for the ConsumerProgress, since the caller wouldn't have the object unless it's been initialized by the factory.

@zhangchiqing zhangchiqing changed the base branch from leo/badger-batch-approvals to master August 23, 2024 01:15
@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 18.55204% with 360 lines in your changes missing coverage. Please review.

Project coverage is 41.44%. Comparing base (400bcd0) to head (161566c).

Files Patch % Lines
storage/badger/operation/reader_batch_writer.go 0.00% 60 Missing ⚠️
storage/mock/badger_reader_batch_writer.go 0.00% 40 Missing ⚠️
storage/badger/operation/common.go 0.00% 30 Missing ⚠️
storage/mock/reader.go 0.00% 29 Missing ⚠️
storage/badger/cache_b.go 59.09% 26 Missing and 1 partial ⚠️
storage/mock/writer.go 0.00% 25 Missing ⚠️
storage/mock/consumer_progress_factory.go 0.00% 23 Missing ⚠️
storage/badger/chunks_queue.go 0.00% 19 Missing ⚠️
storage/badger/consumer_progress.go 0.00% 19 Missing ⚠️
storage/pebble/consumer_progress.go 0.00% 17 Missing ⚠️
... and 13 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6391      +/-   ##
==========================================
- Coverage   41.50%   41.44%   -0.06%     
==========================================
  Files        2013     2021       +8     
  Lines      143577   143867     +290     
==========================================
+ Hits        59590    59631      +41     
- Misses      77813    78061     +248     
- Partials     6174     6175       +1     
Flag Coverage Δ
unittests 41.44% <18.55%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zhangchiqing zhangchiqing marked this pull request as ready for review August 27, 2024 17:31
@zhangchiqing zhangchiqing requested review from jordanschalm and AlexHentschel and removed request for ramtinms August 27, 2024 17:32
@zhangchiqing zhangchiqing changed the base branch from master to leo/badger-batch-approvals August 27, 2024 17:33
storage/consumer_progress.go Outdated Show resolved Hide resolved
storage/consumer_progress.go Show resolved Hide resolved
storage/consumer_progress.go Show resolved Hide resolved
@zhangchiqing
Copy link
Member Author

Close for now. Found a better way to refactor.

@zhangchiqing
Copy link
Member Author

Will re-implement once #6466 is merged.

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.

4 participants