Skip to content

feat(rust/driver_manager): implement connection profiles#3973

Open
zeroshade wants to merge 22 commits intoapache:mainfrom
zeroshade:rust-connection-profile
Open

feat(rust/driver_manager): implement connection profiles#3973
zeroshade wants to merge 22 commits intoapache:mainfrom
zeroshade:rust-connection-profile

Conversation

@zeroshade
Copy link
Member

Implementing connection profiles for the Rust driver manager including the default FilesystemProfileProvider and updating the objects and tests accordingly.

Copy link
Contributor

@felipecrv felipecrv left a comment

Choose a reason for hiding this comment

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

Not a full review yet. But it's a good start.

Comment on lines 561 to 567
drv = ManagedDriver::load_from_name(
driver,
entrypoint,
version,
load_flags,
additional_search_paths.clone(),
)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can become

DriverLibrary::search(name, load_flags, additional_search_paths)?

and you can make every branch return SearchHit. Then at the end you can:

        let entrypoint = search_hit.resolve_entrypoint(entrypoint).to_vec();
        Self::load_from_library(search_hit.library, entrypoint.as_ref(), version)

Then, at the end of all this, you can extract a function that returns SearchHit to search.rs and keep the load_from_library call here. This is what enables different users of search.rs (which I intend to make more public in the upstream too) to instantiate structs that are not ManagedDriver.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is not a trivial change, but you get the intention: move search logic to search.rs and keep only the final instantiation of ManagedDriver in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

that doesn't necessarily work in the case where the profile provides an init_fn though in which case we use load_static rather than using a SearchHit. So i'm not sure how your suggestion would work in that case

@zeroshade
Copy link
Member Author

@felipecrv implemented all the suggestions other than one that I responded to

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