crepererum commented on code in PR #630:
URL:
https://github.com/apache/arrow-rs-object-store/pull/630#discussion_r3026863510
##########
src/local.rs:
##########
@@ -16,13 +16,11 @@
// under the License.
//! An object store implementation for a local filesystem
+#[cfg(feature = "xattr")]
+use std::borrow::Cow;
Review Comment:
Could you inline this import into `set_xattrs`? That's somewhat of a
personal choice, but I find feature-guarded code always easier to reason about
if it doesn't spill all over the place.
##########
Cargo.toml:
##########
@@ -51,45 +51,42 @@ form_urlencoded = { version = "1.2", optional = true }
http-body-util = { version = "0.1.2", optional = true }
httparse = { version = "1.8.0", default-features = false, features = ["std"],
optional = true }
hyper = { version = "1.2", default-features = false, optional = true }
-md-5 = { version = "0.11.0", default-features = false, optional = true }
+md-5 = { version = "0.10.6", default-features = false, optional = true }
quick-xml = { version = "0.39.0", features = ["serialize",
"overlapped-lists"], optional = true }
-rand = { version = "0.10", default-features = false, features = ["std",
"std_rng", "thread_rng"], optional = true }
+rand = { version = "0.9", default-features = false, features = ["std",
"std_rng", "thread_rng"], optional = true }
reqwest = { version = "0.12", default-features = false, features =
["rustls-tls-native-roots", "http2"], optional = true }
ring = { version = "0.17", default-features = false, features = ["std"],
optional = true }
rustls-pki-types = { version = "1.9", default-features = false, features =
["std"], optional = true }
serde = { version = "1.0", default-features = false, features = ["derive"],
optional = true }
serde_json = { version = "1.0", default-features = false, features = ["std"],
optional = true }
serde_urlencoded = { version = "0.7", optional = true }
+tokio = { version = "1.29.0", features = ["sync", "macros", "rt", "time",
"io-util"] }
-# Optional tokio feature
-tokio = { version = "1.29.0", features = ["sync", "macros", "rt", "time",
"io-util"], optional = true }
-tracing = { version = "0.1", optional = true }
+[target.'cfg(target_family="unix")'.dependencies]
+xattr = { version = "1", optional = true }
[target.'cfg(target_family="unix")'.dev-dependencies]
-nix = { version = "0.31.1", features = ["fs"] }
+nix = { version = "0.30.0", features = ["fs"] }
[target.'cfg(all(target_arch = "wasm32", target_os = "unknown"))'.dependencies]
web-time = { version = "1.1.0" }
wasm-bindgen-futures = "0.4.18"
-futures-channel = {version = "0.3", features = ["sink"]}
[features]
Review Comment:
Isn't this missing the `xattr` feature?
##########
src/local.rs:
##########
@@ -882,9 +982,15 @@ impl MultipartUpload for LocalUpload {
async fn complete(&mut self) -> Result<PutResult> {
let src = self.src.take().ok_or(Error::Aborted)?;
let s = Arc::clone(&self.state);
+ let attributes = std::mem::take(&mut self.attributes);
maybe_spawn_blocking(move || {
// Ensure no inflight writes
let file = s.file.lock();
+
+ if !attributes.is_empty() {
+ set_xattrs(&src, &attributes)?;
+ }
Review Comment:
As far as I can see, you already handle the `is_empty` case within
`set_xattrs` so this check is redundant. I think having the check only in one
place (= `set_xattr`) makes the code easier to maintain and reason about.
##########
src/local.rs:
##########
@@ -31,12 +29,14 @@ use std::{collections::VecDeque, path::PathBuf};
use async_trait::async_trait;
use bytes::Bytes;
use chrono::{DateTime, Utc};
-use futures_util::{FutureExt, TryStreamExt};
-use futures_util::{StreamExt, stream::BoxStream};
+use futures::{FutureExt, TryStreamExt};
+use futures::{StreamExt, stream::BoxStream};
use parking_lot::Mutex;
use url::Url;
use walkdir::{DirEntry, WalkDir};
+#[cfg(feature = "xattr")]
Review Comment:
I think this one is wrong: the `#[cfg(not(feature = "xattr"))]` code uses
this import.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
