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

Add ReplicaStore #668

Merged
merged 1 commit into from
Mar 9, 2022
Merged

Add ReplicaStore #668

merged 1 commit into from
Mar 9, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Mar 5, 2022

We can reuse the AuthorityStore for the store in a replica.
There can be more differences but the major difference would be storing all history versions of objects.
This PR adds a type template to the store, so that we could use it for both authority and replica.
Since it's type template, the cost should be minimum.

Open to suggestions on using a boolean flag field instead of type template if there is a good reason.

@lxfind lxfind changed the title [RFC] Add ReplicaStore Add ReplicaStore Mar 7, 2022
@gdanezis
Copy link
Collaborator

gdanezis commented Mar 7, 2022

Love the approach of using a type template!

But instead of adding a new table for all objects, rather could we not just not run the following two lines in the store?
https://github.com/MystenLabs/fastnft/blob/c72a4aea98dc4fe76de8515fe94f0023524829ef/sui_core/src/authority/authority_store.rs#L429-L430

@lxfind
Copy link
Contributor Author

lxfind commented Mar 7, 2022

@gdanezis That table is keyed on ObjectID. So we would still end up overwriting objects.

velvia
velvia previously requested changes Mar 7, 2022
Copy link
Contributor

@velvia velvia left a comment

Choose a reason for hiding this comment

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

Added hopefully useful comments 😄

Regarding one table vs two tables: The only way I could see a unified single table would be a table based on (ObjectID, Version). The Version field would be ignored or set to 0 always in the authority version of the store -- OR you use the right version, but delete the older versions. That would create more churn though, but it might be OK since it fits the LSM pattern.

Actually I kinda like that idea slightly better than two tables. The difference in behaviour would then be if you kept older versions of objects. But maybe it would mess up the read patterns in authorities.


/// AUTHORITY is used to distinguish how the SuiDataStore is being used, whether for
/// the authorities or for replicas.
pub struct SuiDataStore<const AUTHORITY: bool> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of calling it authority, can we instead make it tied to the function which differentiates the store itself - ie the all object versions map. So I'd call it ALL_OBJ_VER: bool instead. The reason is that down the line, another user of the store could need this functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking about that this flag can be used down the line, but in a different way: I think there will be other differences between authorities and non-authorities, not just whether the object versions are kept.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, that is true - my point mainly was that "Authority" might become a misnomer down the line as then the functionality of the store becomes unclear, then I have to look up what differences an authority might have. We can always add more types for diff features, though that adds, well, more type complexity.

/// This is a map between the object ID and the latest state of the object, namely the
/// state that is needed to process new transactions. If an object is deleted its entry is
/// removed from this map.
objects: DBMap<ObjectID, Object>,

/// Stores all history versions of all objects.
/// This is not needed by an authority, but is needed by a replica.
all_object_versions: DBMap<ObjectRef, Object>,
Copy link
Contributor

Choose a reason for hiding this comment

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

An ObjectRef contains the ObjectID, version, and Object Ref.
Isn't the object ref redundant here? There should never be two objects in the system with the same version but different object refs correct?

I would create a new type which is just the ObjectID and version, or just use that as a tuple. Dropping the object ref saves a ton of space in the key, enabling faster object lookups and less space used.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still worried about this part. An ObjectID is 20 bytes, sequence number 28 bytes. An Object digest, the last part of an ObjectRef, is 32 bytes. If we don't need the ObjectRef, that shaves the key size down to less than half of what an entire ObjectRef is. When you multiply this by many many millions of objects it makes a big difference.

@gdanezis
Copy link
Collaborator

gdanezis commented Mar 7, 2022

@gdanezis That table is keyed on ObjectID. So we would still end up overwriting objects.

What about changing its key to ObjectRef?

@lxfind
Copy link
Contributor Author

lxfind commented Mar 8, 2022

Re: using one table instead of two tables

Why do we prefer one over two here? Is it just for code clarity?
There are quite some consequences of merging them to 1 table. For instance, for shared objects, we only have an object ID in the input. If we only have a table keyed by ObjectRef, we would need another table to map from object ID to objectRef.
We could consider setting a dummy version for authorities in this merged table, but the complexity of dealing with that is definitely more than having two tables, unless I am missing something.

@gdanezis
Copy link
Collaborator

gdanezis commented Mar 8, 2022

I do not want to block this forever, but just to clarify what the DBMap BTree allows us to do efficiently:

If we only have a table keyed by ObjectRef, we would need another table to map from object ID to objectRef

We can efficiently lookup the latest ObjectID in in a table indexed by ObjectRef by using the .skip_prev_to() method and a key (ObjectID, u64::MAX, Digest::MAX). This is the reason why we use the compound keys with ObjectID being the first element.

Copy link
Collaborator

@gdanezis gdanezis left a comment

Choose a reason for hiding this comment

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

Lets start somehwere, and this is a fine place to start -- the refactor in the direction we want.

@velvia
Copy link
Contributor

velvia commented Mar 8, 2022

Would shared objects not be stored in the all_object_versions table?

The tradeoffs for two vs one table are:

  1. Size and storage needs
  2. consistency, potential for differences

For a thin client 1) is not a concern, maybe for a full replica that wants to keep both tables it could be a concern, but the size of the all_objects table would quickly outgrow the other one, so probably not.

I'm more worried about 2) since there are many more places we have to get it right. I believe you use batches which is a good start though.

@lxfind
Copy link
Contributor Author

lxfind commented Mar 8, 2022

Let me iterate on this a bit

@lxfind lxfind force-pushed the auth-store-history-flag branch from 6713dbb to d51cd84 Compare March 9, 2022 03:10
@lxfind
Copy link
Contributor Author

lxfind commented Mar 9, 2022

Made the following changes:

  1. Renamed the type template to ALL_OBJ_VER to be specific. We can get back to the naming if we need more different tables.
  2. The all_object_versions table is indexed on (ObjectID, SequenceNumber), which saves some space.

I am still using two tables instead of one. I think that trying to unify them has too many implications to the implementation of AuthorityStore that would complicate the authority implementation. For example, having to delete the old version as well as adding the new version is going to be much more expensive/slower than simply overriding the table by id.
We can try to simplify things down the road if there is a need.

@lxfind lxfind merged commit 2f50ab5 into main Mar 9, 2022
@lxfind lxfind deleted the auth-store-history-flag branch March 9, 2022 04:02
@velvia
Copy link
Contributor

velvia commented Mar 9, 2022

@lxfind thanks for the changes

mwtian pushed a commit that referenced this pull request Sep 12, 2022
mwtian pushed a commit to mwtian/sui that referenced this pull request Sep 29, 2022
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