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

GetEntryOptions #932

Merged
merged 35 commits into from
Feb 5, 2019
Merged

GetEntryOptions #932

merged 35 commits into from
Feb 5, 2019

Conversation

maackle
Copy link
Member

@maackle maackle commented Jan 29, 2019

Question:

How do we deal with the difference between storing one's own header vs. storing others' headers?

  • During Action::Commit, the header gets added to the chain CAS.
  • During Action::Hold, the entry and header get added to the DHT CAS, and metadata is added to DHT EAV

The question is do we also want to add our own header to the DHT CAS and EAV, so we can get them all from the same place? Or do we want to look in both places, adding our own header to the collection of stored headers from other agents? (This is because in general we don't Hold our own Entry)

Answer:

We look in both places: search our chain for our own header to this entry, and also check the DHT meta storage for other headers we've seen, merging the two results into the total set of headers.

  • I have added a summary of my changes to the changelog
  • Add headers to CAS and EAV when seen during Hold
  • Add headers to GetEntryOptions
  • Understand the difference between AgentState.chain.content_storage and DhtStore.content_storage

changes

  • header "sources" is now "provenances"
  • added triple provenance app spec test
    • added carol
    • carolpeletier
  • adds pretty_assertions crate as a core dep
  • Action::Hold now operates on EntryWithHeader
  • chain header no longer optional in validation package
  • DHT can find headers by checking meta/CAS
  • DHT can add headers to CAS/meta
  • fix flaky waiter logic in testing that made invalid assumptions about memory network

followups

@maackle maackle changed the title [WIP] GetEntryOptions GetEntryOptions Jan 30, 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.

Cool! This looks great.

I would prefer looking also in our chain to find our own data locally, instead of either just relying on getting it from a remote DHT node (which would also be fine) or storing a copy in the local DHT store. The latter doesn't make sense this copy would be only for us since the network would not know we have this entry in our DHT..

@zippy
Copy link
Member

zippy commented Feb 3, 2019

Hey @maackle I'd love to merge this but now it's a got a conflict. Tried to merge but not clued in enough on waiter... :-)

@@ -25,7 +25,7 @@ use std::fmt::Debug;
pub type Entity = Address;

/// Using String for EAV attributes (not e.g. an enum) keeps it simple and open
pub type Attribute = String;
pub type Attribute = String; // BUT we totally could make it an enum with a Custom variant!
Copy link
Contributor

Choose a reason for hiding this comment

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

sure, just like Entry ;)

@@ -95,6 +95,12 @@ impl AgentState {
.iter_type(&self.top_chain_header(), &entry.entry_type())
.find(|h| h.entry_address() == &entry.address())
}

pub fn get_header_for_entry_address(&self, entry_address: &Address) -> Option<ChainHeader> {
Copy link
Contributor

Choose a reason for hiding this comment

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

get_first_header_for_entry_address?

there can be many headers for a single entry, even within a single chain

also i think we used to have this but i removed it because it boils down to applying a find to an iter that already exists

also self.chain().iter(&self.top_chain_header()) feels wrong somehow :/

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, it was a real bug.

I introduced iter_chain to AgentState for this purpose, which just iterates from the top header, what do you think?

context: context.clone(),
address: entry.address(),
context: context,
address: entry_wh.entry.address(),
Copy link
Contributor

Choose a reason for hiding this comment

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

don't collapse names of variables like this, write the whole thing out :P

Copy link
Member Author

Choose a reason for hiding this comment

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

How about we deal with this after the ChainItem refactor?

// get the header addresses
.map(|eavi| eavi.value())
// fetch the header content from CAS
.map(|a| self.content_storage().read().unwrap().fetch(&a))
Copy link
Contributor

Choose a reason for hiding this comment

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

any way to not unwrap here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@thedavidmeister Didn't we agree that it's OK to unwrap when locking a mutex or rwlock? All the unwraps are of that variety.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to defer on all the unwraps until we get our error handling story straight (#949)

&ENTRY_HEADER_ATTRIBUTE.to_string(),
&header.address(),
)?;
self.content_storage().write().unwrap().add(header)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

unwrap

&header.address(),
)?;
self.content_storage().write().unwrap().add(header)?;
self.meta_storage().write().unwrap().add_eavi(&eavi)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

unwrap

@@ -137,6 +137,8 @@ pub struct EntryWithMeta {
pub maybe_crud_link: Option<Address>,
}

pub type EntryWithMetaWithHeaders = (EntryWithMeta, Vec<ChainHeader>);
Copy link
Contributor

Choose a reason for hiding this comment

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

😨

Copy link
Member Author

Choose a reason for hiding this comment

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

haha, luckily that never got used

@lucksus lucksus merged commit 9f0e375 into develop Feb 5, 2019
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