Adds Instrument Mode for InstrumentedObjectStore in datafusion-cli #18000
Conversation
- 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
| impl FromStr for InstrumentedObjectStoreMode { | ||
| type Err = DataFusionError; | ||
|
|
||
| fn from_str(s: &str) -> std::result::Result<Self, Self::Err> { |
There was a problem hiding this 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)
| #[derive(Debug)] | ||
| struct InstrumentedObjectStore { | ||
| inner: Arc<dyn ObjectStore>, | ||
| instrument_mode: AtomicU8, |
There was a problem hiding this 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.
There was a problem hiding this comment.
maybe an AtomicBool would make sense given it only has two states 🤔
Minor detail however
There was a problem hiding this comment.
Yes! ...but -- The actual intent here is to have this actually end up looking like:
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.
| #[derive(Debug)] | ||
| struct InstrumentedObjectStore { | ||
| inner: Arc<dyn ObjectStore>, | ||
| instrument_mode: AtomicU8, |
There was a problem hiding this comment.
maybe an AtomicBool would make sense given it only has two states 🤔
Minor detail however
|
Thanks @BlakeOrth |
2 participants
Which issue does this PR close?
This does not fully close, but is an incremental building block component for:
datafusion-cli] Add a way to see what object store requests are made #17207Functionally 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:
Rationale for this change
This is another bite-sized building block on the road to completing #17207
What changes are included in this PR?
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)