Skip to content

Commit

Permalink
Decouple S3FileSystem and S3FuseFileSystem constructors (#1100)
Browse files Browse the repository at this point in the history
<!--
The title and description of pull requests will be used when creating a
squash commit to the base branch (usually `main`).
Please keep them both up-to-date as the code change evolves, to ensure
that the commit message is useful for future readers.
-->

## Description of change

Refactor `S3FuseFileSystem::new` to accept an `S3FileSystem` instance
rather than creating a new one. The change highlights that
`S3FuseFileSystem` is only a wrapper for `S3FileSystem` and will make it
easier to modify `S3FileSystem` construction in future changes.

## Does this change impact existing behavior?

No, it's only a minor internal refactor.

## Does this change need a changelog entry in any of the crates?

No.

---

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and I agree to the terms of
the [Developer Certificate of Origin
(DCO)](https://developercertificate.org/).

Signed-off-by: Alessandro Passaro <[email protected]>
  • Loading branch information
passaro authored Nov 5, 2024
1 parent c3277ef commit a54596b
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 25 deletions.
11 changes: 4 additions & 7 deletions mountpoint-s3/examples/fs_benchmark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use clap::{Arg, ArgAction, Command};
use fuser::{BackgroundSession, MountOption, Session};
use mountpoint_s3::fuse::S3FuseFilesystem;
use mountpoint_s3::prefetch::default_prefetch;
use mountpoint_s3::S3FilesystemConfig;
use mountpoint_s3::{S3Filesystem, S3FilesystemConfig};
use mountpoint_s3_client::config::{EndpointConfig, S3ClientConfig};
use mountpoint_s3_client::S3CrtClient;
use mountpoint_s3_crt::common::rust_log_adapter::RustLogAdapter;
Expand Down Expand Up @@ -169,12 +169,9 @@ fn mount_file_system(
mountpoint.to_str().unwrap()
);
let prefetcher = default_prefetch(runtime, Default::default());
let session = Session::new(
S3FuseFilesystem::new(client, prefetcher, bucket_name, &Default::default(), filesystem_config),
mountpoint,
&options,
)
.expect("Should have created FUSE session successfully");
let fs = S3Filesystem::new(client, prefetcher, bucket_name, &Default::default(), filesystem_config);
let session = Session::new(S3FuseFilesystem::new(fs), mountpoint, &options)
.expect("Should have created FUSE session successfully");

BackgroundSession::new(session).expect("Should have started FUSE session successfully")
}
10 changes: 5 additions & 5 deletions mountpoint-s3/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,16 @@ use nix::unistd::ForkResult;
use regex::Regex;
use sysinfo::{RefreshKind, System};

use crate::build_info;
use crate::data_cache::{CacheLimit, DiskDataCache, DiskDataCacheConfig, ExpressDataCache, ManagedCacheDir};
use crate::fs::{CacheConfig, S3FilesystemConfig, ServerSideEncryption, TimeToLive};
use crate::fs::{CacheConfig, ServerSideEncryption, TimeToLive};
use crate::fuse::session::FuseSession;
use crate::fuse::S3FuseFilesystem;
use crate::logging::{init_logging, prepare_log_file_name, LoggingConfig};
use crate::mem_limiter::MINIMUM_MEM_LIMIT;
use crate::prefetch::{caching_prefetch, default_prefetch, Prefetch};
use crate::prefix::Prefix;
use crate::s3::S3Personality;
use crate::{autoconfigure, metrics};
use crate::{autoconfigure, build_info, metrics, S3Filesystem, S3FilesystemConfig};

const CLIENT_OPTIONS_HEADER: &str = "Client options";
const MOUNT_OPTIONS_HEADER: &str = "Mount options";
Expand Down Expand Up @@ -928,9 +927,10 @@ where
Prefetcher: Prefetch + Send + Sync + 'static,
{
tracing::trace!(?filesystem_config, "creating file system");
let fs = S3FuseFilesystem::new(client, prefetcher, bucket_name, prefix, filesystem_config);
let fs = S3Filesystem::new(client, prefetcher, bucket_name, prefix, filesystem_config);
let fuse_fs = S3FuseFilesystem::new(fs);
tracing::debug!(?fuse_session_config, "creating fuse session");
let session = Session::new(fs, &fuse_session_config.mount_point, &fuse_session_config.options)
let session = Session::new(fuse_fs, &fuse_session_config.mount_point, &fuse_session_config.options)
.context("Failed to create FUSE session")?;
let session = FuseSession::new(session, fuse_session_config.max_threads).context("Failed to start FUSE session")?;

Expand Down
13 changes: 2 additions & 11 deletions mountpoint-s3/src/fuse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ use std::time::SystemTime;
use time::OffsetDateTime;
use tracing::{field, instrument, Instrument};

use crate::fs::{DirectoryEntry, DirectoryReplier, InodeNo, S3Filesystem, S3FilesystemConfig, ToErrno};
use crate::fs::{DirectoryEntry, DirectoryReplier, InodeNo, S3Filesystem, ToErrno};
use crate::prefetch::Prefetch;
use crate::prefix::Prefix;
#[cfg(target_os = "macos")]
use fuser::ReplyXTimes;
use fuser::{
Expand Down Expand Up @@ -76,15 +75,7 @@ where
Client: ObjectClient + Clone + Send + Sync + 'static,
Prefetcher: Prefetch,
{
pub fn new(
client: Client,
prefetcher: Prefetcher,
bucket: &str,
prefix: &Prefix,
config: S3FilesystemConfig,
) -> Self {
let fs = S3Filesystem::new(client, prefetcher, bucket, prefix, config);

pub fn new(fs: S3Filesystem<Client, Prefetcher>) -> Self {
Self { fs }
}
}
Expand Down
10 changes: 8 additions & 2 deletions mountpoint-s3/tests/common/fuse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use mountpoint_s3::data_cache::DataCache;
use mountpoint_s3::fuse::S3FuseFilesystem;
use mountpoint_s3::prefetch::{Prefetch, PrefetcherConfig};
use mountpoint_s3::prefix::Prefix;
use mountpoint_s3::S3FilesystemConfig;
use mountpoint_s3::{S3Filesystem, S3FilesystemConfig};
use mountpoint_s3_client::config::S3ClientAuthConfig;
use mountpoint_s3_client::types::{ObjectPart, PutObjectParams};
use mountpoint_s3_client::ObjectClient;
Expand Down Expand Up @@ -110,7 +110,13 @@ where

let prefix = Prefix::new(prefix).expect("valid prefix");
let session = Session::new(
S3FuseFilesystem::new(client, prefetcher, bucket, &prefix, filesystem_config),
S3FuseFilesystem::new(S3Filesystem::new(
client,
prefetcher,
bucket,
&prefix,
filesystem_config,
)),
mount_dir,
&options,
)
Expand Down

0 comments on commit a54596b

Please sign in to comment.