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

feat(env): patchable environments, named timelines #119

Merged
merged 20 commits into from
Apr 20, 2024
Merged

Conversation

gluax
Copy link
Contributor

@gluax gluax commented Apr 16, 2024

  • improved concurrency for adding new environments, agents
  • env prepare now can add/remove new nodes without restarting the entire environment automatically restarts agents with updated peers
  • the default env is default. this means running snops-cli env prepare ./spec/my-test.yaml will automatically prepare the default env

@gluax gluax self-assigned this Apr 16, 2024
@gluax gluax changed the base branch from main to feat/more-scli-cmds April 16, 2024 17:01
Base automatically changed from feat/more-scli-cmds to main April 18, 2024 01:19
@Meshiest Meshiest marked this pull request as ready for review April 19, 2024 05:18
@Meshiest Meshiest changed the title Feat/named timelines feat(env): patchable environments, named timelines Apr 19, 2024
crates/snops-cli/src/commands/env.rs Outdated Show resolved Hide resolved
crates/snops/src/env/mod.rs Outdated Show resolved Hide resolved
crates/snops/src/env/mod.rs Show resolved Hide resolved
pool: &'a HashMap<AgentId, Agent>,
) -> impl Iterator<Item = &'a Agent> + 'a {
pool: &'a DashMap<AgentId, Agent>,
) -> impl Iterator<Item = dashmap::mapref::one::Ref<'a, AgentId, Agent>> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dashmap::mapref::one::Ref impls Deref<Target = Agent>, so you should be able to make the iterator item a &'a Agent again if you return &*pool.get(&id)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried your suggestion and a few other methods for getting an &Agent but none of them worked out.

  • &*pool.get(&id) -> can't deref an Option
  • pool.get(&id).map(|r| &*r) -> returns a value referencing data owned by the current function
  • pool.get(&id).map(|a| a.value()) -> same as ^

@@ -576,24 +653,28 @@ pub async fn initial_reconcile(env_id: EnvId, state: &GlobalState) -> Result<(),
let not_me = |agent: &AgentPeer| !matches!(agent, AgentPeer::Internal(candidate_id, _) if *candidate_id == id);

node_state.peers = env
.matching_nodes(&node.peers, &pool_lock, PortType::Node)
.matching_nodes(&node.peers, &state.pool, PortType::Node)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is a non-issue but part of the problem of not keeping a lock on the whole pool as you make these changes sequentially is that you can introduce race conditions where other tasks can modify the pool state in between operations. Locking the whole pool for the start of this function did not have that problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer a lock for only agents within the given environment or this approach to globally locking any agents from making changes for an unrelated timeline locking unrelated agents out of state updates

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding locks on the pool is always instantaneous; locks are never held over asynchronously executing code. DashMaps introduce scary new deadlock possibilities since locking is implicit and handled internally

crates/snops/src/server/api.rs Outdated Show resolved Hide resolved
@Meshiest Meshiest merged commit 8d0dc6c into main Apr 20, 2024
2 checks passed
@Meshiest Meshiest deleted the feat/named-timelines branch April 20, 2024 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants