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

[chore] move caches to a separate State{} structure #1078

Conversation

NyaaaWhatsUpDoc
Copy link
Member

This moves toward consolidating much of our means of dependency injection. It will also make it easier sharing resources across the codebase, and in the particular case of the caches, invalidating the necessary caches on particular changes.

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc marked this pull request as draft November 18, 2022 20:48
internal/state/cache.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tsmethurst tsmethurst left a comment

Choose a reason for hiding this comment

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

looks like it's going in a good direction!

is it worth making state.State into an interface instead of just using the struct, so that in future we can use different kinds of state objects? (just thinking years in the future here, when we start messing about with remote state etc)

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc force-pushed the chore/move-caches-to-state-singleton branch from d1b379c to d0001c3 Compare November 19, 2022 22:55
@NyaaaWhatsUpDoc NyaaaWhatsUpDoc force-pushed the chore/move-caches-to-state-singleton branch 2 times, most recently from 95d1431 to 9ed9f31 Compare November 23, 2022 21:57
@NyaaaWhatsUpDoc
Copy link
Member Author

NyaaaWhatsUpDoc commented Nov 23, 2022

@tsmethurst I started to make the State{} itself an interface, but it complicates being able to be granular about initiating individual parts without having tonnes of interface methods. Instead what I went for was making each individual attached piece being an interface. The DB obviously already is, so I updated the caches to now be interfaces which could allow swapping them out for a multi-node compatible deployment in the future.

I would say this is ready to review now, but getting this 0.6.0 release out is more important first for sure :D

@tsmethurst
Copy link
Contributor

lmk when you want me to review this again kimbe :)

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc force-pushed the chore/move-caches-to-state-singleton branch from 5210e12 to dd75419 Compare December 7, 2022 22:25
@NyaaaWhatsUpDoc NyaaaWhatsUpDoc marked this pull request as ready for review December 7, 2022 22:28
Copy link
Contributor

@tsmethurst tsmethurst left a comment

Choose a reason for hiding this comment

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

I had a few comments but nothing significant. This looks really great, and is good to go afaict :) Amazing work kimbe

internal/cache/util.go Show resolved Hide resolved
internal/cache/util.go Show resolved Hide resolved
internal/db/bundb/status.go Show resolved Hide resolved
internal/state/state.go Show resolved Hide resolved
internal/state/state.go Show resolved Hide resolved
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
@NyaaaWhatsUpDoc NyaaaWhatsUpDoc force-pushed the chore/move-caches-to-state-singleton branch from 7b4fb2b to abaa418 Compare December 8, 2022 17:29
@tsmethurst tsmethurst merged commit e58d2d8 into superseriousbusiness:main Dec 8, 2022
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