refactor(puffin): adjust generic parameters #4279
Conversation
Signed-off-by: Zhenchi <[email protected]>
zhongzc
requested review from
evenyag,
v0y4g3r,
waynexia and
a team
as code owners
WalkthroughThe recent code changes primarily involve refactoring the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (9)
- src/mito2/src/sst/index/puffin_manager.rs (6 hunks)
- src/puffin/Cargo.toml (1 hunks)
- src/puffin/src/puffin_manager.rs (2 hunks)
- src/puffin/src/puffin_manager/file_accessor.rs (1 hunks)
- src/puffin/src/puffin_manager/fs_puffin_manager.rs (2 hunks)
- src/puffin/src/puffin_manager/fs_puffin_manager/reader.rs (5 hunks)
- src/puffin/src/puffin_manager/fs_puffin_manager/writer.rs (4 hunks)
- src/puffin/src/puffin_manager/stager.rs (3 hunks)
- src/puffin/src/puffin_manager/stager/bounded_stager.rs (2 hunks)
Files skipped from review due to trivial changes (2)
- src/puffin/Cargo.toml
- src/puffin/src/puffin_manager/file_accessor.rs
Additional comments not posted (24)
src/puffin/src/puffin_manager/fs_puffin_manager.rs (5)
23-23: Approved: Necessary import statement forPuffinFileAccessor.This import is required for the new generic parameter
Fin theFsPuffinManagerstruct and its implementation.
25-25: Approved: Necessary import statement forStager.This import is required for the new generic parameter
Sin theFsPuffinManagerstruct and its implementation.
29-33: Approved: Generic parameters improve flexibility and reusability.The update to use generic parameters
SandFin theFsPuffinManagerstruct enhances its flexibility and reusability.
36-41: Approved: Updated constructor forFsPuffinManager.The new constructor using the generic parameters
SandFensures proper initialization of theFsPuffinManager.
47-55: Approved: UpdatedPuffinManagerimplementation forFsPuffinManager.The updated
PuffinManagerimplementation using the new generic parametersSandFensures proper functionality.src/puffin/src/puffin_manager/stager.rs (1)
55-56: Approved: Enhanced flexibility and thread safety forStagertrait.The
#[auto_impl::auto_impl(Box, Rc, Arc)]annotation allows theStagertrait to be automatically implemented forBox,Rc, andArc. The addition ofSendandSyncbounds ensures thread safety.src/puffin/src/puffin_manager.rs (3)
75-75: Approved: Enhanced flexibility forPuffinReadertrait.The
#[auto_impl::auto_impl(Box, Rc, Arc)]annotation allows thePuffinReadertrait to be automatically implemented forBox,Rc, andArc.
95-95: Approved: Enhanced flexibility forBlobGuardtrait.The
#[auto_impl::auto_impl(Box, Rc, Arc)]annotation allows theBlobGuardtrait to be automatically implemented forBox,Rc, andArc.
103-103: Approved: Enhanced flexibility forDirGuardtrait.The
#[auto_impl::auto_impl(Box, Rc, Arc)]annotation allows theDirGuardtrait to be automatically implemented forBox,Rc, andArc.src/puffin/src/puffin_manager/fs_puffin_manager/writer.rs (5)
33-33: Approved: Necessary import statement forStager.This import is required for the new generic parameter
Sin theFsPuffinWriterstruct and its implementation.
Line range hint
37-50:
Approved: Generic parameters improve flexibility and reusability.The update to use generic parameters
SandWin theFsPuffinWriterstruct enhances its flexibility and reusability.
Line range hint
51-61:
Approved: Updated constructor forFsPuffinWriter.The new constructor using the generic parameters
SandWensures proper initialization of theFsPuffinWriter.
Line range hint
63-157:
Approved: UpdatedPuffinWriterimplementation forFsPuffinWriter.The updated
PuffinWriterimplementation using the new generic parametersSandWensures proper functionality.
Line range hint
166-181:
Approved: Updatedhandle_compressmethod forFsPuffinWriter.The new
handle_compressmethod using the generic parametersSandWensures proper functionality.src/puffin/src/puffin_manager/fs_puffin_manager/reader.rs (5)
19-19: Imports look good.The added imports are necessary for the changes in the struct and its implementation.
Also applies to: 28-31
34-42: Struct changes look good.The changes in the
FsPuffinReaderstruct to use generic parametersSandFare in line with the PR objective.
Line range hint
45-62:
Implementation changes look good.The changes in the
FsPuffinReaderimplementation to support the new generic parameters are necessary and correct.
95-104: Method changes look good.The changes in the
init_blob_to_cachemethod to support the new generic parameters are necessary and correct.
129-129: Method changes look good.The changes in the
init_dir_to_cachemethod to support the new generic parameters are necessary and correct.src/mito2/src/sst/index/puffin_manager.rs (3)
24-24: Imports look good.The added imports are necessary for the changes in the file.
39-40: Type declaration changes look good.The changes in the type declaration to use the new generic parameters are in line with the PR objective.
101-101: New struct addition looks good.The addition of the
ObjectStorePuffinFileAccessorstruct is necessary to support the changes in the file.src/puffin/src/puffin_manager/stager/bounded_stager.rs (2)
428-428: BlobGuard implementation changes look good.The changes in the implementation of the
BlobGuardtrait to remove theArcwrapper are in line with the PR objective.
463-463: DirGuard implementation changes look good.The changes in the implementation of the
DirGuardtrait to remove theArcwrapper are in line with the PR objective.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4279 +/- ##
==========================================
- Coverage 84.96% 84.69% -0.27%
==========================================
Files 1058 1061 +3
Lines 188055 188107 +52
==========================================
- Hits 159780 159317 -463
- Misses 28275 28790 +515 |
|
I wonder if we can change diff --git a/src/mito2/src/sst/index/puffin_manager.rs b/src/mito2/src/sst/index/puffin_manager.rs
index f953ca5dd4..18cfd7f07a 100644
--- a/src/mito2/src/sst/index/puffin_manager.rs
+++ b/src/mito2/src/sst/index/puffin_manager.rs
@@ -36,8 +36,7 @@ type InstrumentedAsyncRead = store::InstrumentedAsyncRead<'static, FuturesAsyncR
type InstrumentedAsyncWrite = store::InstrumentedAsyncWrite<'static, FuturesAsyncWriter>;
pub(crate) type BlobReader = <Arc<FsBlobGuard> as BlobGuard>::Reader;
-pub(crate) type SstPuffinManager =
- FsPuffinManager<Arc<BoundedStager>, ObjectStorePuffinFileAccessor>;
+pub(crate) type SstPuffinManager = FsPuffinManager<BoundedStager, ObjectStorePuffinFileAccessor>;
const STAGING_DIR: &str = "staging";
@@ -71,7 +70,8 @@ impl PuffinManagerFactory {
pub(crate) fn build(&self, store: ObjectStore) -> SstPuffinManager {
let store = InstrumentedStore::new(store).with_write_buffer_size(self.write_buffer_size);
let puffin_file_accessor = ObjectStorePuffinFileAccessor::new(store);
- SstPuffinManager::new(self.stager.clone(), puffin_file_accessor)
+ let stager = self.stager.clone();
+ SstPuffinManager::new(stager, puffin_file_accessor)
}
}
diff --git a/src/puffin/src/puffin_manager/fs_puffin_manager.rs b/src/puffin/src/puffin_manager/fs_puffin_manager.rs
index 7c95532dc6..e9c3854e98 100644
--- a/src/puffin/src/puffin_manager/fs_puffin_manager.rs
+++ b/src/puffin/src/puffin_manager/fs_puffin_manager.rs
@@ -16,6 +16,8 @@ mod dir_meta;
mod reader;
mod writer;
+use std::sync::Arc;
+
use async_trait::async_trait;
pub use reader::FsPuffinReader;
pub use writer::FsPuffinWriter;
@@ -28,14 +30,18 @@ use crate::puffin_manager::PuffinManager;
/// `FsPuffinManager` is a `PuffinManager` that provides readers and writers for puffin data in filesystem.
pub struct FsPuffinManager<S, F> {
/// The stager.
- stager: S,
+ stager: Arc<S>,
/// The puffin file accessor.
puffin_file_accessor: F,
}
-impl<S, F> FsPuffinManager<S, F> {
+impl<S, F> FsPuffinManager<S, F>
+where
+ S: Stager,
+ F: PuffinFileAccessor + Clone,
+{
/// Creates a new `FsPuffinManager` with the specified `stager` and `puffin_file_accessor`.
- pub fn new(stager: S, puffin_file_accessor: F) -> Self {
+ pub fn new(stager: Arc<S>, puffin_file_accessor: F) -> Self {
Self {
stager,
puffin_file_accessor,
@@ -46,7 +52,7 @@ impl<S, F> FsPuffinManager<S, F> {
#[async_trait]
impl<S, F> PuffinManager for FsPuffinManager<S, F>
where
- S: Stager + Clone,
+ S: Stager,
F: PuffinFileAccessor + Clone,
{
type Reader = FsPuffinReader<S, F>;
diff --git a/src/puffin/src/puffin_manager/fs_puffin_manager/reader.rs b/src/puffin/src/puffin_manager/fs_puffin_manager/reader.rs
index a1b8d3a8ea..2430d5d2c3 100644
--- a/src/puffin/src/puffin_manager/fs_puffin_manager/reader.rs
+++ b/src/puffin/src/puffin_manager/fs_puffin_manager/reader.rs
@@ -12,6 +12,8 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+use std::sync::Arc;
+
use async_compression::futures::bufread::ZstdDecoder;
use async_trait::async_trait;
use futures::future::BoxFuture;
@@ -36,14 +38,14 @@ pub struct FsPuffinReader<S, F> {
puffin_file_name: String,
/// The stager.
- stager: S,
+ stager: Arc<S>,
/// The puffin file accessor.
puffin_file_accessor: F,
}
impl<S, F> FsPuffinReader<S, F> {
- pub(crate) fn new(puffin_file_name: String, stager: S, puffin_file_accessor: F) -> Self {
+ pub(crate) fn new(puffin_file_name: String, stager: Arc<S>, puffin_file_accessor: F) -> Self {
Self {
puffin_file_name,
stager,
diff --git a/src/puffin/src/puffin_manager/fs_puffin_manager/writer.rs b/src/puffin/src/puffin_manager/fs_puffin_manager/writer.rs
index ab7227606d..945cbbc236 100644
--- a/src/puffin/src/puffin_manager/fs_puffin_manager/writer.rs
+++ b/src/puffin/src/puffin_manager/fs_puffin_manager/writer.rs
@@ -14,6 +14,7 @@
use std::collections::HashSet;
use std::path::PathBuf;
+use std::sync::Arc;
use async_compression::futures::bufread::ZstdEncoder;
use async_trait::async_trait;
@@ -39,7 +40,7 @@ pub struct FsPuffinWriter<S, W> {
puffin_file_name: String,
/// The stager.
- stager: S,
+ stager: Arc<S>,
/// The underlying `PuffinFileWriter`.
puffin_file_writer: PuffinFileWriter<W>,
@@ -49,7 +50,7 @@ pub struct FsPuffinWriter<S, W> {
}
impl<S, W> FsPuffinWriter<S, W> {
- pub(crate) fn new(puffin_file_name: String, stager: S, writer: W) -> Self {
+ pub(crate) fn new(puffin_file_name: String, stager: Arc<S>, writer: W) -> Self {
Self {
puffin_file_name,
stager,
diff --git a/src/puffin/src/puffin_manager/stager.rs b/src/puffin/src/puffin_manager/stager.rs
index fd2c09254c..0ec565c1c1 100644
--- a/src/puffin/src/puffin_manager/stager.rs
+++ b/src/puffin/src/puffin_manager/stager.rs
@@ -52,7 +52,6 @@ pub trait InitDirFn = Fn(DirWriterProviderRef) -> WriteResult;
/// `Stager` manages the staging area for the puffin files.
#[async_trait]
-#[auto_impl::auto_impl(Box, Rc, Arc)]
pub trait Stager: Send + Sync {
type Blob: BlobGuard;
type Dir: DirGuard; |
Signed-off-by: Zhenchi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/puffin/src/puffin_manager.rs (2 hunks)
- src/puffin/src/puffin_manager/file_accessor.rs (1 hunks)
- src/puffin/src/puffin_manager/stager.rs (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/puffin/src/puffin_manager.rs
- src/puffin/src/puffin_manager/file_accessor.rs
- src/puffin/src/puffin_manager/stager.rs
What benefits can be gained from this? |
Just make the code simpler 😄 overall lgtm |
Signed-off-by: Zhenchi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/puffin/src/puffin_manager.rs (2 hunks)
- src/puffin/src/puffin_manager/file_accessor.rs (1 hunks)
- src/puffin/src/puffin_manager/stager.rs (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/puffin/src/puffin_manager.rs
- src/puffin/src/puffin_manager/file_accessor.rs
- src/puffin/src/puffin_manager/stager.rs
A known issue, fixed in #4283 |
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
#4266 (comment)
What's changed and what's your intention?
reduce generic parameters for
FsPuffinManagerChecklist
Summary by CodeRabbit
Refactor
SstPuffinManagerand related components for better type handling and efficiency.PuffinReader,BlobGuard,DirGuard, andStagertraits withauto_implto support automatic implementation forArctypes.PuffinFileAccessorto enhance compatibility and performance.Dependencies
auto_implversion 1.2.0 to the project dependencies.