feat: Add P2P collection topic subscription #1086

fredcarle · 2023-02-08T09:13:22Z

Relevant issue(s)

Resolves #1081

Description

This PR adds the option to subscribe to events on a given collection by using the collectionID (in reality schemaID for now as there is no globalSchemaID or anything similar) as the pubsub topic. It adds commands to the CLI to add, remove and list the P2P collection topics.

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?

unit tests and integration tests

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

MacOS

codecov · 2023-02-08T09:20:24Z

Codecov Report

Merging #1086 (e2efce2) into develop (5591416) will decrease coverage by 0.77%.
The diff coverage is 27.56%.

@@             Coverage Diff             @@
##           develop    #1086      +/-   ##
===========================================
- Coverage    68.27%   67.50%   -0.77%     
===========================================
  Files          173      178       +5     
  Lines        16338    16616     +278     
===========================================
+ Hits         11154    11216      +62     
- Misses        4253     4455     +202     
- Partials       931      945      +14

Impacted Files	Coverage Δ
db/errors.go	`0.00% <0.00%> (ø)`
net/api/client/client.go	`0.00% <0.00%> (ø)`
net/api/service.go	`2.77% <0.00%> (-1.39%)`	⬇️
cli/p2p_collection_getall.go	`6.06% <6.06%> (ø)`
cli/p2p_collection_add.go	`7.69% <7.69%> (ø)`
cli/p2p_collection_remove.go	`7.69% <7.69%> (ø)`
net/peer.go	`47.65% <23.52%> (-3.76%)`	⬇️
db/p2p_collection.go	`60.00% <60.00%> (ø)`
net/server.go	`59.93% <62.50%> (+0.64%)`	⬆️
core/key.go	`86.52% <73.68%> (-0.93%)`	⬇️
... and 5 more

AndrewSisley

Mostly looks good to me :) And I like what you did to the tests. I do think there is a destructive race condition though that could corrupt the persisted p2pcollection data and I would like the public interface documentation to be expanded.

AndrewSisley · 2023-02-10T16:58:58Z

tests/integration/net/state/utils.go

 		require.NoError(t, err)
+	}
+
+	docInNodes := mapNodesToDoc(test)


praise: Took me a while to think it over, and whilst I generally dislike merging of two different concepts in the same block (here the create/update, and the wait mechanics), I think the new code is easier to understand and harder to break (particularly important with wg stuff where breaking might deadlock) - thanks for this change :)

Thanks Andy :)

tests/integration/net/state/utils.go

AndrewSisley · 2023-02-10T17:11:28Z

client/db.go

@@ -51,6 +51,13 @@ type DB interface {
 	// GetAllReplicators returns the full list of replicators with their
 	// subscribed schemas.
 	GetAllReplicators(ctx context.Context) ([]Replicator, error)
+
+	// AddP2PCollection adds a P2P collection to the stored list


todo: I do not know what the stored list means here, I can guess, but as this is a public interface I think this needs to be explained further. (same for the other new funcs in here).

todo: I think this comment should also highlight that the item to be stored must be a collection id, and that it will error if it is not an id of an existing collection. (same for RemoveP2PCollection)

done ✅

Let me know if you like the change.

AndrewSisley · 2023-02-10T17:14:07Z

client/db.go

+	// RemoveP2PCollection removes P2P collection from the stored list
+	RemoveP2PCollection(ctx context.Context, collectionID string) error
+	// GetAllP2PCollections returns the full list of P2P collections
+	GetAllP2PCollections(ctx context.Context) ([]string, error)


thought: DB is growing quite a bit, and the P2P funcs are starting to outnumber core database funcs - it might be nice to break them out to a different (child?) object, or perhaps just split the interface into two (one core, one P2P, both implemented by db.db). Really doesn't have to be done now if we want to do this, just something to bear in mind).

I created an embedded P2P interface. Let me know if you like that change.

Nice - thanks :) I like it.

Further thought for future (not now) - I dont think the DB interface should inherit the P2P one, instead there should be 3:

P2P {...} DB {...} Defra{ //Naming is horrible, and giving this and DB meaningful names may be tricky DB P2P }

This would allow consumers to properly segregate their use cases if they want (also really useful for mocking, if so inclined). That way only their P2P code has access to the P2P interface, and core code the core DB stuff. Probs less important in Go as interfaces are not explicitly linked, but still nice I think (plus DB inheriting from P2P is a bit odd longer-term).

AndrewSisley · 2023-02-10T17:16:31Z

client/db.go

+	AddP2PCollection(ctx context.Context, collectionID string) error
+	// RemoveP2PCollection removes P2P collection from the stored list
+	RemoveP2PCollection(ctx context.Context, collectionID string) error
+	// GetAllP2PCollections returns the full list of P2P collections


todo: I do not think this comment explains the return value well enough - it is just a string array and nothing in the signature or current comment states which values it contains P2P collections are presumably not just arbitrary strings, and it could easily be read as meaning []client.Collection.

done ✅

Let me know what you think.

looks good - thanks :)

AndrewSisley · 2023-02-10T17:42:56Z

db/p2p_collections.go

+)
+
+// AddP2PCollection adds a P2P collection to the stored list
+func (db *db) AddP2PCollection(ctx context.Context, collectionID string) error {


todo: It looks like there is a race condition in these functions - you dont appear to be protecting against concurrent mutations that are committed between fetching the existing collections, and saving the user defined values - it also looks like this would be potentially destructive, with invalid values being persisted into the database (e.g. the added collection is deleted between the 2 lines, or another actor concurrently updates the persisted value with conflicting changes).

I think a shared mutex (lol...) would solve the worst of this fairly easily (but won't fully protect against conflicting changes, I think that would have to be done in user-space without a more fancy means of persisting these values).

It would also be good to have tests for this, but it might be too painful to do so at this point.

done ✅

I've changed it to a per collection ID key. No more overstepping. Let me know what you think.

Much much nicer than a mutex lol - thanks 😁 Should make Schema Update stuff much easier too.

AndrewSisley · 2023-02-10T17:52:44Z

db/p2p_collections.go

+}
+
+// RemoveP2PCollection removes P2P collection from the stored list
+func (db *db) RemoveP2PCollection(ctx context.Context, collectionID string) error {


comment: I think it is easy to miss this with Schema Updates, this should be updated if/when collections are deleted. I'll note this on the Schema Update ticket. I cant spot an obvious way to make this linkage/requirement stronger/more obvious though.

Good idea to keep track of it.

Changed the storing mechanics from a single key with an array to each collectionID having it’s own key.

AndrewSisley

Looks good, and I really like the recent changes (they are better than the suggestions/thoughts I posted) :)

Relevant issue(s) Resolves #1081 Description This PR adds the option to subscribe to events on a given collection by using the collectionID (in reality schemaID for now as there is no globalSchemaID or anything similar) as the pubsub topic. It adds commands to the CLI to add, remove and list the P2P collection topics.

Relevant issue(s) Resolves sourcenetwork#1081 Description This PR adds the option to subscribe to events on a given collection by using the collectionID (in reality schemaID for now as there is no globalSchemaID or anything similar) as the pubsub topic. It adds commands to the CLI to add, remove and list the P2P collection topics.

fredcarle added 2 commits February 7, 2023 21:14

WIP

25f2846

add P2P collection topic subscription

5bb57d2

fredcarle added feature New feature or request area/collections Related to the collections system 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 Feb 8, 2023

fredcarle added this to the DefraDB v0.5 milestone Feb 8, 2023

fredcarle requested a review from a team February 8, 2023 09:13

fredcarle self-assigned this Feb 8, 2023

revert unecessary change

f8da5b9

fredcarle added 5 commits February 8, 2023 11:48

add struct documentation

ccb4abf

change net testing utils

1f17e7d

remove coupling between core and test

cac82e9

revert change in handleBroadcastLoop

1c4281e

more reverting

6b4ca0e

AndrewSisley requested changes Feb 10, 2023

View reviewed changes

fredcarle added 5 commits February 10, 2023 15:41

remove potential race condition

848a0b1

Changed the storing mechanics from a single key with an array to each collectionID having it’s own key.

apply feedback

dbd156a

apply feedback

c4cfc5c

update p2p collection tests

07d8c5c

fix lint

e2efce2

AndrewSisley approved these changes Feb 10, 2023

View reviewed changes

fredcarle merged commit 5881187 into develop Feb 11, 2023

fredcarle deleted the fredcarle/feat/I1081-p2p-collection-evt branch February 11, 2023 06:26

jsimnz mentioned this pull request Feb 14, 2023

Split the DB interface by concerns #1090

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add P2P collection topic subscription #1086

feat: Add P2P collection topic subscription #1086

fredcarle commented Feb 8, 2023

codecov bot commented Feb 8, 2023 •

edited

Loading

AndrewSisley left a comment

AndrewSisley Feb 10, 2023

fredcarle Feb 10, 2023

AndrewSisley Feb 10, 2023

fredcarle Feb 10, 2023

AndrewSisley Feb 10, 2023

fredcarle Feb 10, 2023

AndrewSisley Feb 10, 2023

AndrewSisley Feb 10, 2023

fredcarle Feb 10, 2023

AndrewSisley Feb 10, 2023

AndrewSisley Feb 10, 2023

fredcarle Feb 10, 2023

AndrewSisley Feb 10, 2023

AndrewSisley Feb 10, 2023

fredcarle Feb 10, 2023

AndrewSisley left a comment

feat: Add P2P collection topic subscription #1086

feat: Add P2P collection topic subscription #1086

Conversation

fredcarle commented Feb 8, 2023

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Feb 8, 2023 • edited Loading

Codecov Report

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

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

codecov bot commented Feb 8, 2023 •

edited

Loading