-
Notifications
You must be signed in to change notification settings - Fork 840
Description
x/sync's Config type is abused as a DI container + a go config pattern - can we abstract over this to avoid leaking the bad implementation into this package? The proof clients are really dependencies of the syncer and theTargetRootis really a parameter for which to start the syncer - I feel like we could probably do something like haveConfigknow about stuff with safe zero values that have reasonable defaults to make tests easier to write + each VM integrating with this won't have to re-define their own defaults (which will all probably be the same). I thinkSimultaneousWorkLimitandStateSyncNodeshave reasonable default values, and the logger can default to not logging. We could also pass the metrics instance through the config and the default can be a new registry - only prod cares to configure to use its own metrics instance.
TargetRooti actually think is not a true config value because as part of state sync we need to define the point to sync to - so I think the caller should be explicitly providing this through a parameter. Similarly I think there aren't reasonable default values for the network clients so those should be defined explicitly as well.
Originally posted by @joshua-kim in #4361
Also discussed offline:
Yeah generally i think x/sync is poorly abstracted, but in my ideal world the x/sync api looks like:
s := NewSyncer(...)
go s.Start()
for {
...
s.UpdateSyncRoot(newRoot)
}
The initial root to sync to in the config shares responsibility with UpdateSyncTarget (two people own defining a sync target) - so I don’t think it belongs in the config
Start could make sense to your point - but if you did that you would want to get rid of UpdateSyncTarget since then those two would share responsibilities. You could do something like Start(…, chan ids.ID) or something and updates to the target root would be provided by the caller (channel is an example of how to provide that information) (e
Metadata
Metadata
Assignees
Labels
Type
Projects
Status