You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "BlakeOrth (via GitHub)" <gi...@apache.org> on 2025/10/09 18:47:57 UTC
[PR] Adds Instrument Mode for InstrumentedObjectStore in datafusion-cli [datafusion]
BlakeOrth opened a new pull request, #18000:
URL: https://github.com/apache/datafusion/pull/18000
## Which issue does this PR close?
This does not fully close, but is an incremental building block component for:
- https://github.com/apache/datafusion/issues/17207
Functionally this code currently does nothing, but it will allow users to change the mode of an instrumented object store through either a CLI flag or with a runtime command.
The full context of how this code is likely to progress can be seen in the POC for this effort:
- https://github.com/apache/datafusion/pull/17266
## Rationale for this change
This is another bite-sized building block on the road to completing #17207
## What changes are included in this PR?
- Adds mode type to allow changing the mode of an InstrumentedObjectStore to datafusion-cli
- Implements string parsing and u8 conversion for InstrumentedObjectStoreMode
- Adds tests to validate trait implementations
## Are these changes tested?
Yes. Unit tests have been added for the trait implementations on this new type.
## Are there any user-facing changes?
No.
cc @alamb
(I'm trying to keep these as small as possible and have them still make sense, however the PRs following this one will likely start to introduce real functionality and may end up being a bit larger)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Adds Instrument Mode for InstrumentedObjectStore in datafusion-cli [datafusion]
Posted by "BlakeOrth (via GitHub)" <gi...@apache.org>.
BlakeOrth commented on code in PR #18000:
URL: https://github.com/apache/datafusion/pull/18000#discussion_r2417737930
##########
datafusion-cli/src/object_storage/instrumented.rs:
##########
@@ -15,36 +15,90 @@
// specific language governing permissions and limitations
// under the License.
-use std::{fmt, sync::Arc};
+use std::{
+ fmt,
+ str::FromStr,
+ sync::{
+ atomic::{AtomicU8, Ordering},
+ Arc,
+ },
+};
use async_trait::async_trait;
-use datafusion::execution::object_store::ObjectStoreRegistry;
+use datafusion::{error::DataFusionError, execution::object_store::ObjectStoreRegistry};
use futures::stream::BoxStream;
use object_store::{
path::Path, GetOptions, GetResult, ListResult, MultipartUpload, ObjectMeta,
ObjectStore, PutMultipartOptions, PutOptions, PutPayload, PutResult, Result,
};
use url::Url;
+/// The profiling mode to use for an [`ObjectStore`] instance that has been instrumented to collect
+/// profiling data. Collecting profiling data will have a small negative impact on both CPU and
+/// memory usage. Default is `Disabled`
+#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
+pub enum InstrumentedObjectStoreMode {
+ /// Disable collection of profiling data
+ #[default]
+ Disabled,
+ /// Enable collection of profiling data
+ Enabled,
+}
+
+impl fmt::Display for InstrumentedObjectStoreMode {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ write!(f, "{:?}", self)
+ }
+}
+
+impl FromStr for InstrumentedObjectStoreMode {
+ type Err = DataFusionError;
+
+ fn from_str(s: &str) -> std::result::Result<Self, Self::Err> {
+ match s.to_lowercase().as_str() {
+ "disabled" => Ok(Self::Disabled),
+ "enabled" => Ok(Self::Enabled),
+ _ => Err(DataFusionError::Execution(format!("Unrecognized mode {s}"))),
+ }
+ }
+}
+
+impl From<u8> for InstrumentedObjectStoreMode {
+ fn from(value: u8) -> Self {
+ match value {
+ 1 => InstrumentedObjectStoreMode::Enabled,
+ _ => InstrumentedObjectStoreMode::Disabled,
+ }
+ }
+}
+
/// Wrapped [`ObjectStore`] instances that record information for reporting on the usage of the
/// inner [`ObjectStore`]
#[derive(Debug)]
struct InstrumentedObjectStore {
inner: Arc<dyn ObjectStore>,
+ instrument_mode: AtomicU8,
Review Comment:
Yes! ...but -- The actual intent here is to have this actually end up looking like:
```rust
pub enum InstrumentedObjectStoreMode {
/// Disable collection of profiling data
#[default]
Disabled,
/// Enable collection of profiling data and output a summary
Summary,
/// Enable collection of profiling data and output all profiling details
Trace,
}
```
Thus why it's an `AtomicU8`. I can change it to a bool until the above is implemented in a PR if that would be preferable though.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Adds Instrument Mode for InstrumentedObjectStore in datafusion-cli [datafusion]
Posted by "BlakeOrth (via GitHub)" <gi...@apache.org>.
BlakeOrth commented on code in PR #18000:
URL: https://github.com/apache/datafusion/pull/18000#discussion_r2417684439
##########
datafusion-cli/src/object_storage/instrumented.rs:
##########
@@ -15,36 +15,90 @@
// specific language governing permissions and limitations
// under the License.
-use std::{fmt, sync::Arc};
+use std::{
+ fmt,
+ str::FromStr,
+ sync::{
+ atomic::{AtomicU8, Ordering},
+ Arc,
+ },
+};
use async_trait::async_trait;
-use datafusion::execution::object_store::ObjectStoreRegistry;
+use datafusion::{error::DataFusionError, execution::object_store::ObjectStoreRegistry};
use futures::stream::BoxStream;
use object_store::{
path::Path, GetOptions, GetResult, ListResult, MultipartUpload, ObjectMeta,
ObjectStore, PutMultipartOptions, PutOptions, PutPayload, PutResult, Result,
};
use url::Url;
+/// The profiling mode to use for an [`ObjectStore`] instance that has been instrumented to collect
+/// profiling data. Collecting profiling data will have a small negative impact on both CPU and
+/// memory usage. Default is `Disabled`
+#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
+pub enum InstrumentedObjectStoreMode {
+ /// Disable collection of profiling data
+ #[default]
+ Disabled,
+ /// Enable collection of profiling data
+ Enabled,
+}
+
+impl fmt::Display for InstrumentedObjectStoreMode {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ write!(f, "{:?}", self)
+ }
+}
+
+impl FromStr for InstrumentedObjectStoreMode {
+ type Err = DataFusionError;
+
+ fn from_str(s: &str) -> std::result::Result<Self, Self::Err> {
Review Comment:
Note that while this is functionally "dead code" in the current implementation, it's important for parsing cli arguments/commands which will come in a follow-on PR "soon" (tm)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Adds Instrument Mode for InstrumentedObjectStore in datafusion-cli [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #18000:
URL: https://github.com/apache/datafusion/pull/18000
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Adds Instrument Mode for InstrumentedObjectStore in datafusion-cli [datafusion]
Posted by "BlakeOrth (via GitHub)" <gi...@apache.org>.
BlakeOrth commented on code in PR #18000:
URL: https://github.com/apache/datafusion/pull/18000#discussion_r2417688211
##########
datafusion-cli/src/object_storage/instrumented.rs:
##########
@@ -15,36 +15,90 @@
// specific language governing permissions and limitations
// under the License.
-use std::{fmt, sync::Arc};
+use std::{
+ fmt,
+ str::FromStr,
+ sync::{
+ atomic::{AtomicU8, Ordering},
+ Arc,
+ },
+};
use async_trait::async_trait;
-use datafusion::execution::object_store::ObjectStoreRegistry;
+use datafusion::{error::DataFusionError, execution::object_store::ObjectStoreRegistry};
use futures::stream::BoxStream;
use object_store::{
path::Path, GetOptions, GetResult, ListResult, MultipartUpload, ObjectMeta,
ObjectStore, PutMultipartOptions, PutOptions, PutPayload, PutResult, Result,
};
use url::Url;
+/// The profiling mode to use for an [`ObjectStore`] instance that has been instrumented to collect
+/// profiling data. Collecting profiling data will have a small negative impact on both CPU and
+/// memory usage. Default is `Disabled`
+#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
+pub enum InstrumentedObjectStoreMode {
+ /// Disable collection of profiling data
+ #[default]
+ Disabled,
+ /// Enable collection of profiling data
+ Enabled,
+}
+
+impl fmt::Display for InstrumentedObjectStoreMode {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ write!(f, "{:?}", self)
+ }
+}
+
+impl FromStr for InstrumentedObjectStoreMode {
+ type Err = DataFusionError;
+
+ fn from_str(s: &str) -> std::result::Result<Self, Self::Err> {
+ match s.to_lowercase().as_str() {
+ "disabled" => Ok(Self::Disabled),
+ "enabled" => Ok(Self::Enabled),
+ _ => Err(DataFusionError::Execution(format!("Unrecognized mode {s}"))),
+ }
+ }
+}
+
+impl From<u8> for InstrumentedObjectStoreMode {
+ fn from(value: u8) -> Self {
+ match value {
+ 1 => InstrumentedObjectStoreMode::Enabled,
+ _ => InstrumentedObjectStoreMode::Disabled,
+ }
+ }
+}
+
/// Wrapped [`ObjectStore`] instances that record information for reporting on the usage of the
/// inner [`ObjectStore`]
#[derive(Debug)]
struct InstrumentedObjectStore {
inner: Arc<dyn ObjectStore>,
+ instrument_mode: AtomicU8,
Review Comment:
This type likely seems like an odd decision in isolation, but it becomes necessary when the mode actually needs to switch behavior in the `ObjectStore` implementation methods.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Adds Instrument Mode for InstrumentedObjectStore in datafusion-cli [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #18000:
URL: https://github.com/apache/datafusion/pull/18000#discussion_r2417725098
##########
datafusion-cli/src/object_storage/instrumented.rs:
##########
@@ -15,36 +15,90 @@
// specific language governing permissions and limitations
// under the License.
-use std::{fmt, sync::Arc};
+use std::{
+ fmt,
+ str::FromStr,
+ sync::{
+ atomic::{AtomicU8, Ordering},
+ Arc,
+ },
+};
use async_trait::async_trait;
-use datafusion::execution::object_store::ObjectStoreRegistry;
+use datafusion::{error::DataFusionError, execution::object_store::ObjectStoreRegistry};
use futures::stream::BoxStream;
use object_store::{
path::Path, GetOptions, GetResult, ListResult, MultipartUpload, ObjectMeta,
ObjectStore, PutMultipartOptions, PutOptions, PutPayload, PutResult, Result,
};
use url::Url;
+/// The profiling mode to use for an [`ObjectStore`] instance that has been instrumented to collect
+/// profiling data. Collecting profiling data will have a small negative impact on both CPU and
+/// memory usage. Default is `Disabled`
+#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
+pub enum InstrumentedObjectStoreMode {
+ /// Disable collection of profiling data
+ #[default]
+ Disabled,
+ /// Enable collection of profiling data
+ Enabled,
+}
+
+impl fmt::Display for InstrumentedObjectStoreMode {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ write!(f, "{:?}", self)
+ }
+}
+
+impl FromStr for InstrumentedObjectStoreMode {
+ type Err = DataFusionError;
+
+ fn from_str(s: &str) -> std::result::Result<Self, Self::Err> {
+ match s.to_lowercase().as_str() {
+ "disabled" => Ok(Self::Disabled),
+ "enabled" => Ok(Self::Enabled),
+ _ => Err(DataFusionError::Execution(format!("Unrecognized mode {s}"))),
+ }
+ }
+}
+
+impl From<u8> for InstrumentedObjectStoreMode {
+ fn from(value: u8) -> Self {
+ match value {
+ 1 => InstrumentedObjectStoreMode::Enabled,
+ _ => InstrumentedObjectStoreMode::Disabled,
+ }
+ }
+}
+
/// Wrapped [`ObjectStore`] instances that record information for reporting on the usage of the
/// inner [`ObjectStore`]
#[derive(Debug)]
struct InstrumentedObjectStore {
inner: Arc<dyn ObjectStore>,
+ instrument_mode: AtomicU8,
Review Comment:
maybe an `AtomicBool` would make sense given it only has two states 🤔
Minor detail however
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Adds Instrument Mode for InstrumentedObjectStore in datafusion-cli [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #18000:
URL: https://github.com/apache/datafusion/pull/18000#issuecomment-3387315262
Thanks @BlakeOrth
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org