re-factor: cleanup snapshot config usage #4629
Conversation
|
If this PR represents a change to the public RPC API:
Thank you for keeping the RPC clients in sync with the server API @JoeySlomowitz. |
JoeySlomowitz
force-pushed
the
cleanup-snapshot-config-usage
branch
6 times, most recently
from
bcfd6b2 to
41ff77c
Compare
resolve call sites more cleanup fix up tests cleanup more cleanup fix compile issue fix waring minor cleanup
JoeySlomowitz
force-pushed
the
cleanup-snapshot-config-usage
branch
from
41ff77c to
7ba47c0
Compare
|
This is a good initiative, thank you. However, to keep this from exploding out of proportion (and getting pretty difficult to test), I suggest you split this PR into two:
|
There was a problem hiding this comment.
Not sure what the benefit of renaming the file is.
There was a problem hiding this comment.
Thanks @alexpyattaev after changing the config struct over to an enum and renaming, snapshot_config felt like a legacy namespace and out of place. I agree there's no huge benefit here, happy to revert if you disagree.
There was a problem hiding this comment.
The main issue with renaming files is that it breaks tools like git blame. But I guess since this is a complete rewrite it does not matter much.
| pub archives_to_retain: NonZeroUsize, | ||
| } | ||
|
|
||
| impl SnapshotStorageConfig { |
There was a problem hiding this comment.
does it make sense to have archives_dir empty? If not, these defaults are not really initializing a valid instance. If yes, it would make sense to document what it means.
| renice_this_thread( | ||
| snapshot_mode | ||
| .get_snapshot_generate_config() | ||
| .unwrap() |
There was a problem hiding this comment.
This unwrap here will likely blow up in the worst possible way. If you are sure this can not happen, use .expect(), otherwise a better way would be to exit the thread and report something like error!("wrong snapshot config") .
| snapshot_config.maximum_full_snapshot_archives_to_retain, | ||
| snapshot_config.maximum_incremental_snapshot_archives_to_retain, | ||
| &snapshot_load_config.full_snapshot_config.archives_dir, | ||
| &incremental_snapshot_config.archives_dir, // FIXME: Should this param be optional too? |
There was a problem hiding this comment.
probably you can split the function into two. One for full, and one for incremental snapshots. In a separate PR.
| snapshot_mode | ||
| .get_snapshot_load_config() | ||
| .incremental_snapshot_config | ||
| .unwrap() |
There was a problem hiding this comment.
this unwrap can explode. need a better way / proof this can never explode.
| pub struct SnapshotLoadOnlyModeConfig { | ||
| pub load_config: SnapshotLoadConfig, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Eq, PartialEq)] | ||
| pub struct SnapshotLoadAndGenerateModeConfig { | ||
| pub load_config: SnapshotLoadConfig, | ||
| pub generate_config: SnapshotGenerateConfig, | ||
| } | ||
|
|
There was a problem hiding this comment.
Since these structs only have a couple of fields, you can probably inline them into SnapshotMode enum to improve readability.
There was a problem hiding this comment.
thank @alexpyattaev just to confirm, do you mean defining it as a tuple like the following?;
pub enum SnapshotMode {
Disabled,
LoadOnly(SnapshotLoadOnlyModeConfig),
LoadAndGenerate(SnapshotLoadConfig, SnapshotGenerateConfig),
}
There was a problem hiding this comment.
You can go further with
pub enum SnapshotMode {
Disabled,
Load(SnapshotLoadConfig),
LoadAndGenerate{load:SnapshotLoadConfig, generate:SnapshotGenerateConfig},
}
Problem
Cleanup task was requested on the old repo here
Summary of Changes
This does a lot of renaming.
Replaced SnapshotConfig struct with SnapshotMode enum, and included more relevant associated types, following the suggestions put forward in the original issue