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

Merge host component key value implementations into new factor crates #2794

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

calebschoepp
Copy link
Collaborator

No description provided.

@lann lann requested a review from rylev August 30, 2024 21:12
Copy link
Collaborator

@rylev rylev 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! Thanks.

I left a few comments about submodule naming. Relatedly, I do wonder if using the word "factor" in the various implementation crates is correct (e.g., factor-key-value-azure). Now that everything is factor based, I'd personally prefer to keep only crates where the Factor trait is implemented with the name "factor". Everything else should drop "factor":

  • factor-key-value-azure => key-value-azure
  • factor-key-value-spin => key-value-spin or key-value-sqlite
  • factor-key-value-redis => key-value-redis

I feel somewhat strongly that this naming scheme is better, so I'd like to block merging just to make sure we have this conversation. However, if others feel differently, I have no issue quickly switching to approve this PR. cc @lann

crates/factor-key-value-azure/src/lib.rs Outdated Show resolved Hide resolved
crates/factor-key-value-redis/src/lib.rs Outdated Show resolved Hide resolved
crates/factor-key-value-spin/src/lib.rs Outdated Show resolved Hide resolved
@lann
Copy link
Collaborator

lann commented Sep 3, 2024

submodule naming

I'm on the fence with this one.

From a bottom-up perspective I get the point about the "factors" label not really applying to e.g. individual KV impls. You could even imagine consuming the Store/StoreManager traits from non-factors code.

However, from a top-down organizational perspective it seems like it might be less confusing - especially to unfamiliar readers - if everything KV-related is under the same spin-factors-key-value-* "namespace".

@calebschoepp
Copy link
Collaborator Author

FWIW I prefer Ryan's suggestion of dropping the factor- prefix. If I were walking into this codebase with fresh eyes I would find it confusing why there are crates with factor- prefixes that aren't actually factors. To me it becomes more clear that these KV crates are dependencies when we remove the factor- prefix. @lann are you happy with me going ahead in this direction?

@calebschoepp
Copy link
Collaborator Author

I think I've addressed all the requested changes and this is ready for a re-review.

@calebschoepp
Copy link
Collaborator Author

I don't have permissions to merge so it'd be great if you could do it @rylev or @lann.

@lann lann merged commit 855572b into fermyon:main Sep 3, 2024
17 checks passed
@calebschoepp calebschoepp deleted the merge-kv-impls branch September 3, 2024 20:50
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.

3 participants