feat: add cli flags for storage capacity per subnetwork #1558
Conversation
| if storage_flag.is_some() && !config.portal_subnetworks.contains(&subnetwork) { | ||
| return Err(Error::raw( | ||
| ErrorKind::ArgumentConflict, | ||
| format!( | ||
| "--storage.{} is set but {subnetwork} subnetwork is not enabled", | ||
| subnetwork.to_cli_arg(), | ||
| ), | ||
| )); | ||
| } |
There was a problem hiding this comment.
I'm not sure we have to error out in this case. I do like the impulse to be strict to avoid accidents.
But I can also imagine a scenario where I'm flipping between enabling and disabling a network, and having to do that in two places in the command being a little annoying.
I'm slightly in favor of dropping this if you're neutral.
| @@ -256,7 +301,7 @@ impl TrinConfig { | |||
|
|
|||
| pub fn new_from<I, T>(args: I) -> Result<Self, clap::Error> | |||
| where | |||
| I: Iterator<Item = T>, | |||
| I: IntoIterator<Item = T>, | |||
There was a problem hiding this comment.
I do really like this change. It also added noise to a big PR. I think it belongs in its own commit/PR.
ethportal-api/src/types/cli.rs
Outdated
| assert_eq!( | ||
| config.storage_capacity_config(), | ||
| StorageCapacityConfig::Specific { | ||
| beacon_mb: Some(0), |
There was a problem hiding this comment.
I don't think there are any actual bugs here, but as a practice the test for setting a specific value should probably pick one that isn't the value you would arrive at by default.
|
Rebased, fixed default capacity tests, added Let me know if you have other comments and I will address them in the followup PR. |
What was wrong?
Based on #1548 and discussion in the last meeting, we agreed that we want following cli flags regarding storage capacity:
storage.totalflag that specifies total storage capacity (in MB)mbshould be set as alias (until we migrate in other places)storage.beacon,storage.historyandstorage.statethat specifies storage capacity (in MB) per subnetwork explicitlySome restrictions:
storage.totalcan't be combined with other flagsstorage.{subnetwork}is specified for one enabled subnetwork, it should be specified for all enabled networksFor future work:
How was it fixed?
TrinConfigis createdPortalStorageConfigTo-Do