Skip to content

Conversation

@BlakeOrth
Copy link
Contributor

Which issue does this PR close?

This does not fully close, but is an incremental building block component to:

Functionally this code currently does nothing, but it's an important component that will allow access to future instrumented object stores. The full context of how this code is likely to progress can be seen in the POC for this effort:

(note: If we need an individual issue to close for process reasons I am happy to make one and associate it with #17207 as part of that larger effort)

Rationale for this change

The full implementation for #17207 is likely too large for a single PR, so to better account for review bandwidth we would like to break the implementation into smaller pieces for easier review. This happens to be about the smallest and least intrusive atomic unit I could think of based on the POC work.

What changes are included in this PR?

  • Adds a new Object Store Registry wrapper to datafusion-cli to support interacting with instrumented object store instances
  • Introduces a new instrumented sub-module to the object_storage module to house all future instrumented object store code
  • Adds basic tests for the new registry wrapper

Are these changes tested?

Yes, new tests have been introduced and all existing tests are passing

Are there any user-facing changes?

No

cc: @alamb

@BlakeOrth BlakeOrth force-pushed the feature/cli_instrumented_registry branch 2 times, most recently from 9494588 to 65c980c Compare October 8, 2025 00:29
 - Adds a new Object Store Registry wrapper to datafusion-cli to support
   interacting with instrumented object store instances
 - Adds basic tests for the new registry wrapper
@BlakeOrth BlakeOrth force-pushed the feature/cli_instrumented_registry branch from 65c980c to dc60619 Compare October 8, 2025 00:29
@alamb
Copy link
Contributor

alamb commented Oct 8, 2025

I will review this later todya -- thank you @BlakeOrth

@alamb
Copy link
Contributor

alamb commented Oct 8, 2025

(note: If we need an individual issue to close for process reasons I am happy to make one and associate it with #17207 as part of that larger effort)

I don't think this is necessary. Linking to the overall ticket is just great

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me -- thank you @BlakeOrth

I'll plan to merge this later today unless others would like a chance to comment

use object_store::ObjectStore;
use url::Url;

#[derive(Debug)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to add some comments here explaining what this structure is good for

Something like

Suggested change
#[derive(Debug)]
/// Returns wrapped [`ObjectStore`] instances that record
/// network requests for reporting.
#[derive(Debug)]

@alamb
Copy link
Contributor

alamb commented Oct 8, 2025

Let's keep development going 🚀

@alamb alamb added this pull request to the merge queue Oct 8, 2025
Merged via the queue into apache:main with commit f8ff82a Oct 8, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants