Apply dyn-cfg to p1 cases #710
Conversation
There was a problem hiding this comment.
This is extremely cursed, love it
There was a problem hiding this comment.
This might be one of the rare cases where it would be good to try and get high unit test coverage
There was a problem hiding this comment.
how about 97.38% for dynamic_toml.go ? (also, thanks, I found a few bugs in it)
| Verif storiface.Verifier | ||
| As *multictladdr.MultiAddressSelector | ||
| Maddrs map[dtypes.MinerAddress]bool | ||
| Maddrs *config.Dynamic[map[dtypes.MinerAddress]bool] |
There was a problem hiding this comment.
I know we like our hammers, but this also really looks like atomic.Value
There was a problem hiding this comment.
There's a subtle difference worth noting: All dynamics are under 1 mutex so that we never get one updated and the other with the original value, even for a short while.
| # Updates will affect running instances. | ||
| # Updates will affect running instances. |
There was a problem hiding this comment.
Weird that this is doubled up
| db: db, | ||
| api: api, | ||
| maxWaitTime: cfg.Ingest.MaxDealWaitTime, | ||
| maxWaitTime: cfg.Ingest.MaxDealWaitTime.Get(), |
There was a problem hiding this comment.
Should either propagate the dynamic deeper or not make it a dynamic in the config.
tasks/f3/f3_task.go
Outdated
|
|
||
| func (f *F3Task) Adder(taskFunc harmonytask.AddTaskFunc) { | ||
| for a := range f.actors { | ||
| for a := range f.actors.Get() { |
There was a problem hiding this comment.
Not really dynamic, this only runs once on startup
There was a problem hiding this comment.
done. That was one of the first ones before I realized what I needed to do. I must not have swept back well enough.
There was a problem hiding this comment.
We have f05 support in Wallet Manager, this is the legacy version of that - I'd just drop it
There was a problem hiding this comment.
DId you want me to remove all of balance manager? It's only 7 months old, are you sure its functions have been superceded?
There was a problem hiding this comment.
yes, seemed mildly interesting at the time, but I have now removed it.
|
We should Test this with UI and see if new encode/decode works with our weird UI setup. |
Make our Ingest & Address configuration dynamic such that the values apply instantly.
Market will ERROR on 0 <--> 1 that a restart is required to enable the correct services.
Discoveries
Computed Values: These become their own config.NewDynamic() which gets updated by a callback on the source(s).
I could be persuaded to turn these into event streams into reactive programming: https://github.com/samber/ro
I don't think we should be stopping & starting services on these changes. That will simply be unsupported.
Bad configuration is setup to throw error and reuse the original in all the secondary-interpretations, otherwise it'll stream out errors until fixed in the direct consumptions. These seem reasonable.
I couldn't get TOML to do value unmarshal as it knows documents only at the top level, so I built an ugly wrapper full of reflection that we should use instead of TOML unmarshal. It works, but it's ugly.