[Store] Allow parameters to be set via cli and env vars#195
Conversation
| self.ChunkDbPath = filepath.Join(path, "chunks") | ||
| if self.ChunkDbPath == "" { | ||
| self.ChunkDbPath = filepath.Join(path, "chunks") | ||
| } |
There was a problem hiding this comment.
Is this the best way to handle this or does it make more sense to add this to NewDefaultStoreParams ? https://github.com/async-la/go-ethereum/blob/872aeef800ee6368704bec7fc8108d4a235b6e19/swarm/storage/netstore.go#L61
There was a problem hiding this comment.
It needs to be here because the default for a path can be overridden by command line or env var later.
christopherdro
force-pushed
the
storage-params
branch
from
872aeef to
3d283ad
Compare
| EnvVar: SWARM_ENV_STORE_CACHE_CAPACITY, | ||
| } | ||
| SwarmStoreRadius = cli.IntFlag{ | ||
| Name: "radius", |
There was a problem hiding this comment.
I think the flags should be prefixed with store., for example:
--store.path
--store.size
--store.cache.size
--store.radius
| } | ||
|
|
||
| if storeDbCapacity := os.Getenv(SWARM_ENV_STORE_DB_CAPACITY); storeDbCapacity != "" { | ||
| if id, _ := strconv.Atoi(storeDbCapacity); id != 0 { |
There was a problem hiding this comment.
This should return an error rather silently ignoring an incorrectly formatted value.
There was a problem hiding this comment.
Strictly for this config or all flags that try and convert string to int?
There was a problem hiding this comment.
I mean there should always be a good reason to ignore errors, and I don't feel like there is a good reason to here (e.g. if I put --store.size=1O (i.e. the letter O rather than a zero) then I'd rather get an error than silently ignore).
So I'd recommend all code like this to check the error, but I don't think it is necessary to make that change in this PR.
There was a problem hiding this comment.
Agreed. I’ll make the changes here and follow up with another PR addressing the others.
There was a problem hiding this comment.
It doesn't look like we need to use os.Getenv at all if we use the EnvVar property when defining our cli flags. https://github.com/ethersphere/go-ethereum/pull/195/files#diff-5be8f583a91eb8ca7024b8af84ef4b28R165 I think this also behaves as intended with the order of precedence being , cli -> env vars -> default.
The most recent changes I see around this was in ethereum/go-ethereum#15548. @holisticode I think we we might be able to get rid of envVarsOverride here as long as the EnvVar property is set when the flags are initially defined.
@lmars By using the correct types for our cli flags, we get syntax checking we want.
Setting --store.size=1O would result in a in the following error.
invalid value "1O" for flag -store.size: strconv.ParseUint: parsing "1O": invalid syntax
There was a problem hiding this comment.
Indeed it seems EnvVar can supply the needs of envVarsOverride - nevertheless before doing so thorough testing is required. I think to remember that EnvVar behavior with default values sometimes interfered with intended behavior, but I don't recall details.
| self.ChunkDbPath = filepath.Join(path, "chunks") | ||
| if self.ChunkDbPath == "" { | ||
| self.ChunkDbPath = filepath.Join(path, "chunks") | ||
| } |
christopherdro
force-pushed
the
storage-params
branch
3 times, most recently
from
04d62f7 to
22c4761
Compare
| SwarmUploadMimeType, | ||
| // pss flags | ||
| SwarmPssEnabledFlag, | ||
| // storage flags |
gbalint
force-pushed
the
swarm-network-rewrite
branch
from
a9cd26e to
ddfc0a2
Compare
|
@gbalint Was there a reason you tried to merge network rewrite here? |
christopherdro
force-pushed
the
storage-params
branch
from
22c4761 to
1b0a051
Compare
|
I just rebased and forced push back up. Should be good to go again. |
gbalint
force-pushed
the
swarm-network-rewrite
branch
from
70d231b to
c38007a
Compare
janos
force-pushed
the
swarm-network-rewrite
branch
from
c38007a to
70d231b
Compare
4 participants
I'm not sure if this is the best branch to PR against but I wanted to avoid future conflicts with #172