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

Use lib3h persistence crates #1505

Merged
merged 58 commits into from Jun 20, 2019
Merged

Use lib3h persistence crates #1505

merged 58 commits into from Jun 20, 2019

Conversation

struktured
Copy link
Contributor

@struktured struktured commented Jun 10, 2019

PR summary

This PR incorporates the new persistence crates migrated from holochain-core and generalized into the lib3h repo.

  • the cas and eav apis are stripped out from holochain_core_types into holochain_persistence_api within holochain-persistence repository
  • core_types_derive is moved into holochain-serialization as holochain_json_derive
  • cas_implementations is removed and factored into holochain_persistence_[mem|file|pickle] in the holochain-persistence repository.
  • attribute validation is done by wrapping calls to EntityAttributeValueIndex::new with a factory function in eavi.rs (previously was done internally).

testing/benchmarking notes

  • existing cas / eav tests still exist
    ( if any manual testing or benchmarking was/should be done, add notes and/or screenshots here )

followups

  • make separate repos for both persistence and json crates.

changelog

Please check one of the following, relating to the CHANGELOG-UNRELEASED.md

  • this is a code change that effects some consumer (e.g. zome developers) of holochain core so it is added to the CHANGELOG-UNRELEASED.md (linked above), with the format - summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)
  • this is not a code change, or doesn't effect anyone outside holochain core development

@struktured struktured self-assigned this Jun 10, 2019
@struktured struktured added the WIP work in progress label Jun 10, 2019
@struktured struktured removed the WIP work in progress label Jun 11, 2019
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.

Woah! I‘m quite surprised by this PR.

  • Why renaming all of this to lib3h_persistence? I thought lib3h would use the same CAS/EAV from core.
  • Why ripping out all the implementations? I thought lib3h would get a trait object in the same way core just gets a trait object from the conductor. That way we keep core clean from file system access code and alike that won’t compile to WASM and confine future compile-time switches to only conductor level.

I would like to talk about the applied strategy in this PR. I‘m pretty sure we can do this (providing storage to lib3h) with much less changes and much less consequences.

@struktured
Copy link
Contributor Author

Also, @AshantiMutinta's changes in #1363 would get lost by this.

I've merged in @AshantiMutinta's changes in 7b703d2 and holochain/lib3h#109

Copy link
Contributor

@StaticallyTypedAnxiety StaticallyTypedAnxiety left a comment

Choose a reason for hiding this comment

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

Happy To See this in, looks good!

Copy link
Contributor

@thedavidmeister thedavidmeister left a comment

Choose a reason for hiding this comment

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

i'm still pretty confused about the idea of "lib3h persistence crates"

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.

I see we have the persistence crates up on crates.io but the source is still in the lib3h repo. I just came back to this because I was about to add a new constructor to the EavFilter, i.e. I was about to make a change to the stuff that gets moved out of this repo here.

I would very much prefer moving this code into its own repo over moving it to the lib3h repo, and I still would prefer leaving it here over moving it out at all.

Suggestion for a compromise: changing the title of this PR to "Moving persistence crates to their own repository" and making it do exactly that. @thedavidmeister, do you think that makes sense? From my perspective it still seems to be a mistake to move this out of this repository at all (at this point at least) since we do have some implicit coupling between the CAS/EAVI implementations and how core works. Progressing core likely leads to changes or additions being needed which then all require a version bump and crates.io publish just for compiling and running CI on it.

@struktured
Copy link
Contributor Author

Woah! I‘m quite surprised by this PR.

  • Why renaming all of this to lib3h_persistence? I thought lib3h would use the same CAS/EAV from core.

The names are now:

  • holochain_json_derive (previously core_types_derive)
  • holochain_json_api (extracted from core_types)
  • holochain_persistence_api (extracted from core_types)
  • holochain_persistence_[file|mem|pickle] (previously cas_implementations)
  • Why ripping out all the implementations? I thought lib3h would get a trait object in the same way core just gets a trait object from the conductor. That way we keep core clean from file system access code and alike that won’t compile to WASM and confine future compile-time switches to only conductor level.

You're right that in essence the trait is the only thing that is needed to be shared and perhaps that would have been a simpler approach to start with. However from a separation of concerns perspective it's beneficial to factorize out the shared implementations and since the work is done we might as well take advantage of it.

As for what repo the code should exist, the ideal is to make separate repositories for persistence and json, but at the moment they live in lib3h for simplicity. If this is an immediate pain point for some, we could easily start migrating the code to the appropriate repos after (or even before) the PR is merged (since everything uses crates.io anyhow).

I would like to talk about the applied strategy in this PR. I‘m pretty sure we can do this (providing storage to lib3h) with much less changes and much less consequences.

@lucksus We had a few overflow discussions to reach agreement- are there any other concerns left on your end?

@struktured
Copy link
Contributor Author

i'm still pretty confused about the idea of "lib3h persistence crates"

Renamed to holochain_persistence_[api|pickle|mem|file]

@zippy zippy dismissed stale reviews from lucksus and thedavidmeister June 19, 2019 20:04

addressed

}

unsafe impl Sync for Attribute {}
Copy link
Member

Choose a reason for hiding this comment

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

why does this have to be unsafe?

@zippy zippy merged commit b0ee302 into develop Jun 20, 2019
@zippy zippy deleted the using-persistence-crates branch October 4, 2019 18:31
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.

None yet

5 participants