Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DynamicHostComponent::validate_app #1455

Merged
merged 2 commits into from May 9, 2023

Conversation

lann
Copy link
Collaborator

@lann lann commented May 5, 2023

Closes #1371
Ref #1453

This replaces stored closures with the common "dyn-safe wrapper" trait
object pattern.

This results in a little more code but simplifies adding future host
component features.

Signed-off-by: Lann Martin <lann.martin@fermyon.com>
@lann lann requested a review from itowlson May 5, 2023 15:56
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

I'm afraid a lot of the Dyns and Handles went over my head, but that's not really an issue from the user point of view, we mortals just have a function to implement and life is good. So that side of things looks great - thanks!

I wanted, though, to flag components may need to change to take advantage of this design. Specifically, the K/V component currently contains basically just a Rust closure that you give it a component and it returns a store manager, which in turn you give it a store name and it returns a store. That is, it doesn't contain a list of of valid stores that validate_app could consult. Although we could still validate a K/V app from this (by trying out every store name in a component's key_value_stores and seeing if any of them error), that has side-effects (every valid non-BYO store gets created even if never used), and presumably takes more time than a string compare (though probably not a noticeable startup delay).

So the question is, are we comfortable having host components include info that is needed only for validation, and specifically having to change the design of an existing host component to accommodate that?

(As ever, apologies if I am missing something that is obvious or that you have already anticipated.)

crates/app/src/host_component.rs Outdated Show resolved Hide resolved
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
@lann lann force-pushed the host-component-validate-app branch from bf3ffa1 to 5aa3449 Compare May 8, 2023 13:36
@lann
Copy link
Collaborator Author

lann commented May 8, 2023

So the question is, are we comfortable having host components include info that is needed only for validation, and specifically having to change the design of an existing host component to accommodate that?

If they need to, sure. This is an optional trait method that just is just intended to improve DX by moving some runtime errors to "deploy" time.

In the case of the K/V component, I think there are a couple of reasonable options:

  • validate_app could call StoreManagerManager::get (but not StoreManager::get) for each configured store, and the StoreManagerManager closure returned in build_key_value_component could perform the validation.

  • Add a method to StoreManager, e.g.:

trait StoreManager {
  ...
  /// Validate that the given store _might_ be accessible.
  /// Should perform only "cheap" validation, i.e. no network or disk I/O.
  fn validate_store(&self, name: &str) -> Result<(), Error> {
    Ok(())
  }
}

@lann lann merged commit 54543d0 into fermyon:main May 9, 2023
9 checks passed
@lann lann deleted the host-component-validate-app branch May 9, 2023 23:43
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.

Add DynamicHostComponent::validate_app
2 participants