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

Db refactor 1 #1091

Closed
wants to merge 6 commits into from
Closed

Db refactor 1 #1091

wants to merge 6 commits into from

Conversation

freesig
Copy link
Contributor

@freesig freesig commented Nov 4, 2021

Summary

This is the first PR in the db refactor series.

  • Remove from_agent from places that doesn't make sense once we merge dbs.
  • Change HolochainP2pCell to HolochailP2pDna`.

There's a few little work arounds just to make this PR work until the next which will remove them.
I will post the next PR once this is merged due to the size and potential for conflicts.

Overall there's a lot of noise from name changes but it's a pretty simple PR.

TODO:

  • CHANGELOG(s) updated with appropriate info
  • Just before pressing the merge button, ensure new entries to CHANGELOG(s) are still under the UNRELEASED heading

Comment on lines +26 to +32

// Temporary workaround until we remove the need for a
// single validator in the next PR.
let validator = match vault.kind() {
DbKind::Cell(id) => id.agent_pubkey().clone(),
_ => unreachable!(),
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Things like this will be removed in the follow up PRs

Comment on lines +141 to +158
todo!()
// ok_fut(Ok(self.sb.share(|sb| {
// let agent = sb
// .node_for_local_agent_hash_mut(&*to_agent)
// .unwrap()
// .local_agent_by_hash_mut(&*to_agent)
// .unwrap();
// for (hash, op_data) in ops {
// let loc8 = hash.get_loc().as_loc8();
// // TODO: allow setting integration status
// agent.ops.insert(
// loc8,
// AgentOpEntry {
// is_integrated: true,
// },
// );
// }
// })))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maackle This is what I was talking to you about. It doesn't make sense to do things like .node_for_local_agent_hash_mut(&*to_agent). Any idea how we can change this?

..
} => {
let from_agent = super::agent_info::agent_info(_ribosome, call_context.clone(), ())?
.agent_latest_pubkey;
Copy link
Contributor

Choose a reason for hiding this comment

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

just curiosity, why is it "latest", does the pubkey iterate somehow in this context?

Copy link
Contributor

Choose a reason for hiding this comment

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

@neonphog yeah there's an "initial" as well

if finished && con.peer_cert() == *endpoint.cert() {
i.initiate_tgt = None;
}
let futs = bloom.inner.share_mut(move |i, _| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we ready to delete simple_bloom yet? If everybody's switched over to the default sharded_gossip, we shouldn't spend cycles maintaining something that isn't needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh sounds like a good idea, maybe in a separate PR?

Copy link
Contributor

@neonphog neonphog left a comment

Choose a reason for hiding this comment

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

couple optional suggestions / questions - looks good

@freesig freesig added the NO-MERGE (dependency) This PR depends on another - and does not point to "develop". label Nov 4, 2021
@freesig
Copy link
Contributor Author

freesig commented Nov 4, 2021

Unfortunately I made a mistake that is a little hard to fix in this PR but will be fixed in a follow up so even though this is approved I won't merge it but instead pull it into Db refactor 2. I have addressed all the suggestions though

This was referenced Nov 15, 2021
@freesig freesig closed this Nov 19, 2021
@jost-s jost-s deleted the db-refactor-1 branch January 18, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO-MERGE (dependency) This PR depends on another - and does not point to "develop".
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants