feat: Persist p2p replicators #960

fredcarle · 2022-12-06T17:50:24Z

Relevant issue(s)

Resolves #462
Resolves #955
Resolves #954
Resolves #952

Description

This PR adds the ability to persist P2P replicators. It change to RPC part of the CLI to have a new sub command which is replicator.

Using that sub command, we can add, delete and getAll to operate on the replicator store.

Note: Using the config file to add replicators will be done in a future PR. Will most likely add integration test before merging this PR.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

manual tests

Specify the platform(s) on which this was tested:

MacOS

codecov · 2022-12-06T18:10:00Z

Codecov Report

Merging #960 (ae48085) into develop (f835e27) will decrease coverage by 2.72%.
The diff coverage is 10.57%.

@@             Coverage Diff             @@
##           develop     #960      +/-   ##
===========================================
- Coverage    59.83%   57.11%   -2.73%     
===========================================
  Files          160      164       +4     
  Lines        17724    19020    +1296     
===========================================
+ Hits         10605    10863     +258     
- Misses        6165     7173    +1008     
- Partials       954      984      +30

Impacted Files	Coverage Δ
net/api/client/client.go	`0.00% <0.00%> (ø)`
net/api/service.go	`4.16% <0.00%> (-4.53%)`	⬇️
net/api/pb/api.pb.go	`0.86% <0.76%> (-0.48%)`	⬇️
cli/replicator_getall.go	`4.54% <4.76%> (ø)`
cli/replicator_delete.go	`11.11% <11.11%> (ø)`
cli/replicator_set.go	`11.53% <11.53%> (ø)`
net/peer.go	`44.01% <21.59%> (+11.82%)`	⬆️
core/crdt/composite.go	`73.33% <72.72%> (-2.73%)`	⬇️
db/replicator.go	`78.82% <78.82%> (ø)`
core/key.go	`87.60% <91.30%> (+0.09%)`	⬆️
... and 11 more

AndrewSisley

I've added a few comments (all fairly localized) - I've not finished the peer.go stuff though as my brain is slowing down and I'm fairly unfamiliar with this part of the codebase

db/replicator.go

client/db.go

net/peer.go

AndrewSisley

Looks good Fred - a few localized comments, but it makes sense (I think 😁).

net/peer.go

AndrewSisley · 2022-12-09T16:07:24Z

client/db.go

@@ -39,6 +40,10 @@ type DB interface {
 	Events() events.Events

 	PrintDump(ctx context.Context) error
+
+	AddReplicator(ctx context.Context, rep Replicator) error


thought: It might be nice if anything persisted via these calls gets automagically picked up by the P2P code (mid run, not just on restart), have you had any thoughts around that?

I'm not sure I understand what you are saying here. When a call to add replicator is received, the replicator is immediately picked up.

It is? It looked like db.AddReplicator only persists the replicator config to store, and the peer code only ever checks the store for saved configs on defra startup?

If it was as you say the tests wouldn't pass :)

😁 Which bit am I misunderstanding? The peer stuff will pick up new stuff in the db (polling?)?

The entry point when adding a replicator is line 280 in net/peer.go. If you start there you'll have a clearer view of what is happening.

Ah but that is what I mean - client.DB is a public interface, and users are free to call these members as and when they like. This function signature suggests (albeit much less so after renaming) that calling Add/SetReplicator would cause it to be active (and picked up auto magically by the peer code).

It would be nice if it did this at some point in the future.

I'm not sure I agree here. The functionality is available from the peer and the method should be called from peer. Nothing guarantees that peer exists and the interactions with the db are only to operate on the datastore.

Ideally these methods would be internal. But that might be a change for future me lol.

😁 That would also solve this lol

AndrewSisley

Actually, I can approve as the only todos I added ask for documentation (if you dont want changes)

db/replicator_test.go

shahzadlone · 2022-12-13T15:14:12Z

db/replicator_test.go

+
+	err = db.AddReplicator(ctx, client.Replicator{
+		Info:    *info,
+		Schemas: []string{"test", "test2", "test3"},


question: If we have an already existing test schema for the same info then it is just ignored or overwritten in a way?

If will be ignored. It's a cumulative add to ensure the user doesn't to remember all schemas that are replicated in order to just add one.

db/replicator_test.go

net/api/pb/Makefile

shahzadlone

Thanks for the changes.

Relevant issue(s) sourcenetwork#462 sourcenetwork#955 sourcenetwork#954 sourcenetwork#952 Description This PR adds the ability to persist P2P replicators. It change to RPC part of the CLI to have a new sub command which is replicator. Using that sub command, we can add, delete and getAll to operate on the replicator store. Note: Using the config file to add replicators will be done in a future PR. Will most likely add integration test before merging this PR.

fredcarle added feature New feature or request area/p2p Related to the p2p networking system action/no-benchmark Skips the action that runs the benchmark. area/cli Related to the CLI binary labels Dec 6, 2022

fredcarle added this to the DefraDB v0.4 milestone Dec 6, 2022

fredcarle requested a review from a team December 6, 2022 17:50

fredcarle self-assigned this Dec 6, 2022

AndrewSisley requested changes Dec 8, 2022

View reviewed changes

db/replicator.go Outdated Show resolved Hide resolved

db/replicator.go Outdated Show resolved Hide resolved

client/db.go Outdated Show resolved Hide resolved

net/peer.go Outdated Show resolved Hide resolved

AndrewSisley reviewed Dec 9, 2022

View reviewed changes

net/peer.go Outdated Show resolved Hide resolved

AndrewSisley requested changes Dec 9, 2022

View reviewed changes

AndrewSisley approved these changes Dec 9, 2022

View reviewed changes

shahzadlone reviewed Dec 13, 2022

View reviewed changes

db/replicator_test.go Outdated Show resolved Hide resolved

shahzadlone reviewed Dec 13, 2022

View reviewed changes

db/replicator_test.go Show resolved Hide resolved

shahzadlone reviewed Dec 13, 2022

View reviewed changes

net/api/pb/Makefile Outdated Show resolved Hide resolved

fredcarle and others added 8 commits December 15, 2022 14:41

add replicator persisting

ee1cd2a

fix address overwrite

0f914cb

fix context cancelled on RPC

467479a

fix readme replicator port

736f3c8

fix lint

75ddf19

add integration test

88d0ea3

Added ObjectMarker checks to CompositeCRDT Merge

2c06df6

change replicator test struct

2ba7fdf

fredcarle force-pushed the fredcarle/feat/I462-persist-p2p-replicators branch from df313d1 to 2ba7fdf Compare December 15, 2022 20:17

fredcarle added 3 commits December 15, 2022 15:41

apply feedback

953cc27

remove superfluous object marker code

0fd5d19

apply feedback

ae48085

shahzadlone approved these changes Dec 15, 2022

View reviewed changes

fredcarle merged commit 0261377 into develop Dec 15, 2022

fredcarle deleted the fredcarle/feat/I462-persist-p2p-replicators branch December 15, 2022 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Persist p2p replicators #960

feat: Persist p2p replicators #960

fredcarle commented Dec 6, 2022 •

edited

Loading

codecov bot commented Dec 6, 2022 •

edited

Loading

AndrewSisley left a comment

AndrewSisley left a comment

AndrewSisley Dec 9, 2022

fredcarle Dec 15, 2022

AndrewSisley Dec 15, 2022

fredcarle Dec 15, 2022

AndrewSisley Dec 15, 2022

fredcarle Dec 15, 2022

AndrewSisley Dec 15, 2022

fredcarle Dec 15, 2022

fredcarle Dec 15, 2022

AndrewSisley Dec 15, 2022

AndrewSisley left a comment

shahzadlone Dec 13, 2022

fredcarle Dec 15, 2022

shahzadlone left a comment

feat: Persist p2p replicators #960

feat: Persist p2p replicators #960

Conversation

fredcarle commented Dec 6, 2022 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Dec 6, 2022 • edited Loading

Codecov Report

AndrewSisley left a comment

Choose a reason for hiding this comment

AndrewSisley left a 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

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

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

fredcarle commented Dec 6, 2022 •

edited

Loading

codecov bot commented Dec 6, 2022 •

edited

Loading