BlakeOrth commented on PR #17266: URL: https://github.com/apache/datafusion/pull/17266#issuecomment-3267229694
@alamb Thanks for the review! I'll take a look into why it's suddenly stopped working (or perhaps it's a "works on my machine" situation, which is also never good). > I think it is possible to break this into a few smaller PRs which might be faster to review: > > 1. Add basic object store instrumentation and plumbing, but only instrument one operation (like get or list), and set the pattern > 2. Other PRs to fill out the rest of the object store methods. I'm happy to split and divide up the work in whatever manner you think will be best for reviews. I know that review bandwidth is almost always strained, so let me know how we can make that process the smoothest and I'm happy to facilitate as much as I can. I will note that splitting up the actual instrumentation of the `object_store` methods might end up being a bit awkward because the different methods communicate somewhat different data. As an example, since `list` returns a stream of futures collecting a `duration` for it (at least in this simple instrumentation) doesn't make much sense because it's effectively instant. `get`, however, can be awaited in the instrumented call and as such the duration is an accurate representation of the duration of the `get` call. I guess my concern here is mostly that the final structure of the instrumented object store and its metadata might not make sense if the context is an individual method. Looking at the changes here, and the comments you left, I can see two easy PRs that can be done immediately to help streamline the implementation. They'd likely be a good first code contribution if any community members are looking for a simple task to pick up! 1. Turn `object_storage.rs` into a directory and named module to prepare for `object_storage/instrumented.rs` to be introduced 2. Implement a builder for `print_options` so it can be better encapsulated and has better ergonomics now that additional options are being added -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
