Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve sharing of configuration context #64

Open
mycognosist opened this issue May 29, 2023 · 3 comments
Open

Improve sharing of configuration context #64

mycognosist opened this issue May 29, 2023 · 3 comments
Labels
refactor Code rewrite for clarity or performance

Comments

@mycognosist
Copy link
Owner

Right now we have a clumsy hybrid of configuration values which are passed down to actors and other functions (starting in src/node.rs) and those which are set and retrieved from OnceCells.

The OnceCell pattern generally makes it easier to deal with the situation where we have to pass a value into an actor in a loop. Generally these values do not implement the Copy trait and are therefore only able to be moved once before the compiler complains:

^^^^^^^ value moved here, in previous iteration of loop

The following values are currently set via OnceCell:

// NETWORK_KEY
config.network.key
// PEERS_TO_REPLICATE
config.replication.peers
// RESYNC_CONFIG
config.replication.resync
// SECRET_CONFIG
config.secret

One option is simply to chuck the whole ApplicationConfig instance into a OnceCell and avoid passing down config values entirely. Such a "global store" is generally discouraged in Rust.

Another option is to create a context which can be shared with all actors. Each actor is then able to access config values from the context.

@mycognosist mycognosist added the refactor Code rewrite for clarity or performance label May 29, 2023
@mycognosist
Copy link
Owner Author

This could be tackled after #63 is complete.

@mycognosist
Copy link
Owner Author

Another option is to create a context which can be shared with all actors. Each actor is then able to access config values from the context.

This is the approach taken by aquadoggo : https://github.com/p2panda/aquadoggo/blob/main/aquadoggo/src/context.rs

The ServiceManager (equivalent of the broker in the solar context) holds the context object and passes it into each new service upon creation : https://github.com/p2panda/aquadoggo/blob/main/aquadoggo/src/manager.rs#L94

@mycognosist
Copy link
Owner Author

mycognosist commented Jun 13, 2023

Something to consider: we may want mutability for some aspects of the shared state. I'm thinking in particular about an address book, the likes of which currently lives in the ReplicationConfig as the peers field. The peers field could be updated to only contain a HashSet of public keys (these are the peers with whom we wish to replication). A separate peer_addresses field could exist in the NetworkConfig as a HashMap (or similar type) of public key -> address mappings. The peer_addresses would need to be mutable so that addresses discovered during node operation can be added and queried.

Solution: use an Arc<Mutex<_>> only on the fields which require mutability (not the whole context object). For example:

https://github.com/p2panda/aquadoggo/blob/c47cfee3f7c5bda120d9c049d92e0f8a1b19d2c5/aquadoggo/src/schema/schema_provider.rs#L12-L22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code rewrite for clarity or performance
Projects
Development

No branches or pull requests

1 participant