Closed zhongzc closed 1 week ago
The recent code changes primarily involve refactoring the puffin_manager.rs
file and related modules to use the auto_impl::auto_impl(Arc)
attribute for automatic Arc
implementations. This includes updating the PuffinReader
, BlobGuard
, DirGuard
, and Stager
traits, as well as modifying type declarations and constructors in SstPuffinManager
. These adjustments streamline the codebase and improve maintainability.
Files | Change Summary |
---|---|
src/mito2/src/sst/index/puffin_manager.rs , src/puffin/src/puffin_manager.rs , src/puffin/src/puffin_manager/file_accessor.rs , src/puffin/src/puffin_manager/stager.rs |
Annotated traits PuffinReader , BlobGuard , DirGuard , PuffinFileAccessor , and Stager with #[auto_impl::auto_impl(Arc)] . Updated trait bounds and type declarations to use Arc . Adjusted constructors and type implementations accordingly. |
src/puffin/Cargo.toml |
Added auto_impl dependency version "1.2.0". |
Tedious lines now melt away,
Arc
advances, here to stay.
auto_impl
guards our keep,
Less to code, more to reap.
Puffin safe in stager's arms,
Grace and speed in all its charms.
🌟🚀
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?
Attention: Patch coverage is 90.90909%
with 1 line
in your changes missing coverage. Please review.
Project coverage is 84.69%. Comparing base (
03c933c
) to head (37e39c6
). Report is 3 commits behind head on main.
I wonder if we can change stager: S
to stager: Arc<S>
so that we don't have to derive Stager
impl for Arc<T>
/Rc<T>
/Box<T>
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;
I wonder if we can change
stager: S
tostager: Arc<S>
so that we don't have to deriveStager
impl forArc<T>
/Rc<T>
/Box<T>
What benefits can be gained from this?
@WenyXu a failed fuzz test, PTAL https://github.com/GreptimeTeam/greptimedb/actions/runs/9792344295/job/27038170891?pr=4279
I wonder if we can change
stager: S
tostager: Arc<S>
so that we don't have to deriveStager
impl forArc<T>
/Rc<T>
/Box<T>
What benefits can be gained from this?
Just make the code simpler 😄 overall lgtm
@WenyXu a failed fuzz test, PTAL https://github.com/GreptimeTeam/greptimedb/actions/runs/9792344295/job/27038170891?pr=4279
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)
https://github.com/GreptimeTeam/greptimedb/pull/4266#discussion_r1665121324
What's changed and what's your intention?
reduce generic parameters for
FsPuffinManager
Checklist
Summary by CodeRabbit
Refactor
SstPuffinManager
and related components for better type handling and efficiency.PuffinReader
,BlobGuard
,DirGuard
, andStager
traits withauto_impl
to support automatic implementation forArc
types.PuffinFileAccessor
to enhance compatibility and performance.Dependencies
auto_impl
version 1.2.0 to the project dependencies.