Allow Poll Lifetime to be configured via CLI #8885
Conversation
|
It looks like @gnunicorn signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
5chdn
added
A0-pleasereview 🤓
parity/cli/mod.rs
Outdated
| @@ -728,6 +728,10 @@ usage! { | |||
| "--gas-price-percentile=[PCT]", | |||
| "Set PCT percentile gas price value from last 100 blocks as default gas price when sending transactions.", | |||
|
|
|||
| ARG arg_poll_lifetime: (u32) = 60u32, or |c: &Config| c.mining.as_ref()?.poll_lifetime.clone(), | |||
| "--poll-lifetime=[S]", | |||
| "Set the lifetime of the internal index filter to S seconds. Deafult: 60", | |||
There was a problem hiding this comment.
Didn't review; just a nitpick: you don't need to specify the default value in the help message; it's automatically appended.
debris
added
A6-mustntgrumble 💦
Great question! As of today, we don't care about versions of crate in this repo. Usually they are set to |
gnunicorn
force-pushed
the
fix-5484-configure-poll-lifetime
branch
from
ef4e79e
to
5f02a23
Compare
... rather than it being a hard-coded constant. fixes openethereum#5484 .
gnunicorn
force-pushed
the
fix-5484-configure-poll-lifetime
branch
from
5f02a23
to
e8ce616
Compare
|
@debris I guess that is okay-ish... or do we know of any other people using this/our internal crates and thus, we'd break their build (as cargo follow semver?)... I mean, there is little downside for us to bump if, especially if it effects many people ... also. the latest tests failures weren't mine but because of an old master. gitlab passed now. |
I don't think these crates are used directly by anyone. Usually people use parity over rpc |
|
@5chdn it doesn't grumble anymore. mind re-add the review-tag? |
debris
added
A8-looksgood 🦄
|
PR is already reviewed well. It just needs someone else to sign off. |
+1, this is not clear to me what this means: #8917 |
* master: ethstore: retry deduplication of wallet file names until success (#8910) Update ropsten.json (#8926) Include node identity in the P2P advertised client version. (#8830) Allow disabling local-by-default for transactions with new config entry (#8882) Allow Poll Lifetime to be configured via CLI (#8885)
…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: Include node identity in the P2P advertised client version. (openethereum#8830) Allow disabling local-by-default for transactions with new config entry (openethereum#8882) Allow Poll Lifetime to be configured via CLI (openethereum#8885) cleanup nibbleslice (openethereum#8915) Hardware-wallets `Clean up things I missed in the latest PR` (openethereum#8890) Remove debian/.deb and centos/.rpm packaging scripts (openethereum#8887) Remove a weird emoji in new_social docs (openethereum#8913) Minor fix in chain supplier and light provider (openethereum#8906)
... rather than it being a hard-coded constant. fixes #5484 .
However, in order to allow this to be configurable, we have to pass through the from the CLI through a range of structs, for which this is unrelated until it finally reaches the internally used PollManager. To achieve this I had to modify, among others,
EthClientandEthClientOptionsboth of which are public structs exposed byparity-rpc. Thus means, these are technically backwards incompatible API-changes, which - under SemVer - would mean we have to bump the Major version of this crate...? Not sure if we want to do that.Secondly, I am not super certain I traced the entire flow everywhere and there isn't still a
::Default()somewhere that creates poll-lifetimes other than what the CLI specifies. Happy for any more experienced reviewer to point them out if I missed any!