In-Memory Snapshot Implementation #204

jordanschalm · 2020-12-03T00:08:55Z

This PR defines an in-memory implementation of protocol.Snapshot and a canonical encoding structure for Snapshot.

Changes

Adds EncodableSnapshot and sub-structures, which define the structure of encoding a snapshot
Adds inmem.Snapshot, a memory-backed implementation of protocol.Snapshot that uses EncodableSnapshot as its underlying data structure
Adds conversion function inmem.FromSnapshot which converts any implementation of protocol.Snapshot to a memory-backed snapshot + smoke tests

…le-snapshot

there are cyclical dependencies otherwise

These were effectively already using similar memory-backed implementations, this removes the duplication

…izable-snapshot

zhangchiqing

Looks good to me

state/protocol/errors.go

utils/unittest/epoch_builder.go

yhassanzadeh13 · 2020-12-09T01:57:23Z

state/protocol/invalid/epoch.go

+}
+
+func (u *Epochs) Next() protocol.Epoch {
+	return NewEpoch(u.err)


can this recursively go through? (e.g., ..Next().Next().Next()), if so, does this make sense to prefix the error each time we create a new epoch?

No there's only one layer of querying. From a block reference (ie state.AtBlockID(id)) we can query only the previous/current/next epochs from that reference. Next returns an Epoch object which doesn't have a Next method.

yhassanzadeh13 · 2020-12-09T01:59:40Z

state/protocol/inmem/convert.go

+		return nil, err
+	}
+	snap.Identities, err = from.Identities(filter.Any)
+	if err != nil {


I would add some prefix to this error letting them get nested: return nil, fmt.Errorf("could not retrieve identities: %w")

Same suggestion for the rest of the errors (unless you want to preserve their type later on).

yhassanzadeh13 · 2020-12-09T02:06:59Z

state/protocol/inmem/convert_test.go

+)
+
+// should be able to convert from protocol.Snapshot
+func TestFromSnapshot(t *testing.T) {


better to start godoc with name of the function // TestFromSnapshot ....

yhassanzadeh13 · 2020-12-09T02:09:46Z

state/protocol/inmem/convert_test.go

+			t.Run("staking phase", func(t *testing.T) {
+				expected := state.AtHeight(1)
+				actual, err := inmem.FromSnapshot(expected)
+				require.Nil(t, err)


you may use require.NoError here as well.

yhassanzadeh13 · 2020-12-09T02:12:01Z

state/protocol/inmem/encodable.go

+	"github.com/onflow/flow-go/model/flow"
+)
+
+// Snapshot is the encoding format for protocol.Snapshot


Suggested change

// Snapshot is the encoding format for protocol.Snapshot

// EncodableSnapshot is the encoding format for protocol.Snapshot

yhassanzadeh13 · 2020-12-09T02:12:42Z

state/protocol/inmem/encodable.go

+	Next     *EncodableEpoch
+}
+
+// Epoch is the encoding format for protocol.Epoch


Suggested change

// Epoch is the encoding format for protocol.Epoch

// EncodableEpoch is the encoding format for protocol.Epoch

yhassanzadeh13 · 2020-12-09T02:13:45Z

state/protocol/inmem/encodable.go

+	DKG               *EncodableDKG
+}
+
+// DKG is the encoding format for protocol.DKG


Suggested change

// DKG is the encoding format for protocol.DKG

// EncodableDKG is the encoding format for protocol.DKG

yhassanzadeh13 · 2020-12-09T02:14:08Z

state/protocol/inmem/encodable.go

+	Participants map[flow.Identifier]flow.DKGParticipant
+}
+
+// Cluster is the encoding format for protocol.Cluster


Suggested change

// Cluster is the encoding format for protocol.Cluster

// EncodableCluster is the encoding format for protocol.Cluster

yhassanzadeh13 · 2020-12-09T02:16:51Z

state/protocol/inmem/snapshot.go

+	enc EncodableSnapshot
+}
+
+func (s *Snapshot) Head() (*flow.Header, error) {


I think these methods are not required to be pointer receiver. They do not change affect s and safer to be by value.

yhassanzadeh13

Clean implementation 👍 left a few comments basically related to the linting

jordanschalm · 2020-12-09T23:09:33Z

Thanks @yhassanzadeh13, applied those suggestions

jordanschalm added 5 commits December 1, 2020 17:57

add serializable models for snapshot

495e910

Merge branch 'master' into jordan/4889-serializable-snapshot

807d325

complete implementation of protocol interfaces

b76ccdf

add encodable models for protocol snapshot

69e35da

use encodable models as base of in-memory snapshot

516ba76

jordanschalm requested review from AlexHentschel, m4ksio, psiemens, ramtinms, yhassanzadeh13 and zhangchiqing as code owners December 3, 2020 00:08

jordanschalm marked this pull request as draft December 3, 2020 00:09

jordanschalm added 6 commits December 2, 2020 16:10

Merge branch 'jordan/4889-qc-in-snapshot' into jordan/4889-serializab…

2286e8a

…le-snapshot

rename package to inmem

410eee3

godocs

4a06296

expose RandomSource in epoch

fae3a2e

add conversion to in-memory snapshot

bc46fab

move encodable models to inmem package

514332a

there are cyclical dependencies otherwise

jordanschalm removed request for m4ksio, zhangchiqing, ramtinms, psiemens, yhassanzadeh13 and AlexHentschel December 3, 2020 02:22

add smoke test for snapshot conversion

2b57e06

jordanschalm self-assigned this Dec 3, 2020

jordanschalm requested review from AlexHentschel, zhangchiqing and yhassanzadeh13 December 3, 2020 02:41

jordanschalm changed the title ~~Serializable In-Memory Snapshot Implementation~~ In-Memory Snapshot Implementation Dec 3, 2020

jordanschalm added 2 commits December 3, 2020 12:45

use inmem models for dkg/cluster

6ce812b

These were effectively already using similar memory-backed implementations, this removes the duplication

make current epoch non-nullable in encodable model

360c02b

Base automatically changed from jordan/4889-qc-in-snapshot to feature/serializable-snapshots December 4, 2020 01:52

jordanschalm added 2 commits December 4, 2020 09:48

Merge branch 'feature/serializable-snapshots' into jordan/4889-serial…

ffed878

…izable-snapshot

Merge branch 'feature/serializable-snapshots' into jordan/4889-serial…

9f69385

…izable-snapshot

jordanschalm mentioned this pull request Dec 4, 2020

FEATURE: Serializable Snapshots #209

Merged

zhangchiqing approved these changes Dec 7, 2020

View reviewed changes

jordanschalm removed the request for review from AlexHentschel December 8, 2020 18:22

yhassanzadeh13 reviewed Dec 9, 2020

View reviewed changes

state/protocol/errors.go Show resolved Hide resolved

yhassanzadeh13 reviewed Dec 9, 2020

View reviewed changes

utils/unittest/epoch_builder.go Outdated Show resolved Hide resolved

yhassanzadeh13 reviewed Dec 9, 2020

View reviewed changes

yhassanzadeh13 approved these changes Dec 9, 2020

View reviewed changes

jordanschalm added 7 commits December 9, 2020 14:30

update error string wording

9009ba7

use NoError in epoch builder

444c0d1

wrap all errors in snapshot conversion

50210b4

use godoc convention for test functions

fae28e5

use NoError over Nil in convert_test.go

9d59fbb

update encodable type names

4812a9c

use non-pointer receiver for inmem.Snapshot

2c7c16c

jordanschalm merged commit 1e3c5c3 into feature/serializable-snapshots Dec 9, 2020

jordanschalm deleted the jordan/4889-serializable-snapshot branch December 9, 2020 23:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In-Memory Snapshot Implementation #204

In-Memory Snapshot Implementation #204

jordanschalm commented Dec 3, 2020 •

edited

Loading

zhangchiqing left a comment

yhassanzadeh13 Dec 9, 2020

jordanschalm Dec 9, 2020 •

edited

Loading

yhassanzadeh13 Dec 9, 2020

yhassanzadeh13 Dec 9, 2020 •

edited

Loading

yhassanzadeh13 Dec 9, 2020

yhassanzadeh13 Dec 9, 2020

yhassanzadeh13 Dec 9, 2020

yhassanzadeh13 Dec 9, 2020

yhassanzadeh13 Dec 9, 2020

yhassanzadeh13 Dec 9, 2020

yhassanzadeh13 Dec 9, 2020

yhassanzadeh13 left a comment

jordanschalm commented Dec 9, 2020

	// Snapshot is the encoding format for protocol.Snapshot
	// EncodableSnapshot is the encoding format for protocol.Snapshot

	// Epoch is the encoding format for protocol.Epoch
	// EncodableEpoch is the encoding format for protocol.Epoch

	// DKG is the encoding format for protocol.DKG
	// EncodableDKG is the encoding format for protocol.DKG

	// Cluster is the encoding format for protocol.Cluster
	// EncodableCluster is the encoding format for protocol.Cluster

In-Memory Snapshot Implementation #204

In-Memory Snapshot Implementation #204

Conversation

jordanschalm commented Dec 3, 2020 • edited Loading

Changes

zhangchiqing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanschalm Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhassanzadeh13 Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhassanzadeh13 left a comment

Choose a reason for hiding this comment

jordanschalm commented Dec 9, 2020

jordanschalm commented Dec 3, 2020 •

edited

Loading

jordanschalm Dec 9, 2020 •

edited

Loading

yhassanzadeh13 Dec 9, 2020 •

edited

Loading