Skip to content
This repository has been archived by the owner on Feb 3, 2023. It is now read-only.

Transactional api integration #2076

Open
wants to merge 33 commits into
base: develop
Choose a base branch
from
Open

Transactional api integration #2076

wants to merge 33 commits into from

Conversation

struktured
Copy link
Contributor

@struktured struktured commented Jan 21, 2020

PR summary

This PR integrates the latest api changes/improvements from this holochain- persistence PR. The PR needs to be merged and release on crates.io BEFORE this PR is merged. Summary of changes:

  • Introduces PersistenceManager trait which wraps a single CAS, EAV, and transactional cursor api.
  • Uses CursorRw to mutate the CAS and EAV in a transactional capacity in the persister and reducers.
  • Removes explicit Attribute implementation as it's provided by persistence api now.

testing/benchmarking notes

( if any manual testing or benchmarking was/should be done, add notes and/or screenshots here )

followups

( any new tickets/concerns that were discovered or created during this work but aren't in scope for review here )

changelog

  • if this is a code change that effects some consumer (e.g. zome developers) of holochain core, then it has been added to our between-release changelog with the format
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)

documentation

@struktured struktured self-assigned this Jan 21, 2020
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.

Just putting a reject on here to make sure it doesn't get merged before the git references in Cargo.tomls get set to real published versions - feel free to clear this when ready.

use holochain_persistence_api::{
error::PersistenceResult,
txn::{CursorDyn, CursorProviderDyn},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer uses at top of file or within a scope

self.holding_map == other.holding_map
&& (*content.read().unwrap()).get_id() == (*other_content.read().unwrap()).get_id()
&& *meta.read().unwrap() == *other_meta.read().unwrap()
&& (*self.persistence_manager).get_id() == (*other.persistence_manager).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 saw another Eq operation like this with the derefs / get_id() thing - should this be a PartialEq/Eq on the persistence_manager itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that this could be a partialEQ on the Persistence Manager itself

@@ -117,28 +106,31 @@ pub fn create_get_links_eavi_query<'a>(
))
}

use holochain_persistence_api::txn::{CursorDyn, CursorProviderDyn};
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto use placement

do
cd $d
if grep "$CRATE" Cargo.toml > /dev/null; then
cargo-add add $CRATE --git $REPO --branch $BRANCH
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

lol... deptool2.. : )

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.

A lot of the code is easier to read/comprehend - nice work 👍

@struktured struktured changed the title WIP: Transactional api integration Transactional api integration Jan 31, 2020
Copy link
Collaborator

@lucksus lucksus left a comment

Choose a reason for hiding this comment

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

👏 👏 👏
(+1 on cleaning up the dependencies after merging the remote branch)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants