refactor: P2P client interface #1924

nasdf · 2023-10-03T20:13:49Z

Relevant issue(s)

Resolves #1883

Description

Blocked by #1927

This PR moves the client.P2P implementation from client.DB to net.Node. This fixes the problems mentioned in the issue above and should increase test coverage of the HTTP and CLI clients.

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?

make test

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

MacOS

…ions in net/peer

…relay in tests

codecov · 2023-10-04T16:32:52Z

Codecov Report

Attention: 225 lines in your changes are missing coverage. Please review.

Comparison is base (5c1b21e) 74.77% compared to head (580f016) 74.24%.

❗ Current head 580f016 differs from pull request most recent head 40b2603. Consider uploading reports for the commit 40b2603 to get more accurate results

@@             Coverage Diff             @@
##           develop    #1924      +/-   ##
===========================================
- Coverage    74.77%   74.24%   -0.53%     
===========================================
  Files          240      236       -4     
  Lines        23644    22879     -765     
===========================================
- Hits         17678    16985     -693     
+ Misses        4759     4738      -21     
+ Partials      1207     1156      -51

Flag	Coverage Δ
all-tests	`74.24% <59.96%> (-0.53%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
cli/p2p_collection_add.go	`100.00% <100.00%> (ø)`
cli/p2p_collection_getall.go	`81.25% <100.00%> (ø)`
cli/p2p_collection_remove.go	`100.00% <100.00%> (ø)`
cli/p2p_info.go	`100.00% <100.00%> (+57.14%)`	⬆️
cli/utils.go	`44.29% <100.00%> (+1.64%)`	⬆️
config/config.go	`73.47% <100.00%> (+0.06%)`	⬆️
db/db.go	`74.10% <100.00%> (+11.69%)`	⬆️
db/txn_db.go	`55.35% <ø> (-2.55%)`	⬇️
http/errors.go	`66.67% <ø> (ø)`
http/handler.go	`96.84% <100.00%> (+0.03%)`	⬆️
... and 15 more

... and 49 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c1b21e...40b2603. Read the comment docs.

AndrewSisley · 2023-10-04T19:26:16Z

remove GRPC API

I really really think that such a change deserves its own PR.

nasdf · 2023-10-04T19:29:48Z

remove GRPC API

I really really think that such a change deserves its own PR.

It's not used in the current develop branch. I had deprecated it when refactoring the CLI and forgot to remove it.

I'm happy to pull those changes into a new PR if you'd prefer.

AndrewSisley

I have a fairly major todo RE transactionality, I'll resume review once that conversation is resolved.

AndrewSisley · 2023-10-04T19:19:57Z

config/config.go

@@ -350,79 +349,38 @@ func (apicfg *APIConfig) AddressToURL() string {

 // NetConfig configures aspects of network and peer-to-peer.
 type NetConfig struct {


praise: Nice 😁 Glad this is simpler now

AndrewSisley · 2023-10-04T19:22:30Z

cli/p2p_replicator_delete.go

 	var cmd = &cobra.Command{
-		Use:   "delete <peer>",
+		Use:   "delete [-c, --collection] <peer>",
 		Short: "Delete a replicator. It will stop synchronizing",


question: Should this (and Long), not now read Delete replicators...? It looks like it can now delete multiple.

If you are referring to deleting multiple collections replicated to a single peer you are correct.

todo: Please change the short/long wording to reflect this :)

AndrewSisley · 2023-10-04T19:36:35Z

client/p2p.go

 type P2P interface {
+	DB


todo: It took me a while to figure it out, but you have removed functionality here. client.P2P used to respect the transactionality of client.Store, but now it only works with implicit transactions.

I'm not sure we wish to lose this, and this change should be made very visible and open to discussion with the team.

I discussed this with @fredcarle and forgot to make a note. We came to the conclusion that peer operations only make sense when done implicitly. It would be pretty difficult to have the transaction also rollback the peer connections that happen during these operations.

We are not losing functionality. We are removing the need for the Store interface to support P2P related actions. The functionality still exists but is handled all within the net package. However, whatever implements the P2P interface needs access to a db instance and thus support the DB interface. Action related to the P2P interface can still use transactions.

We came to the conclusion that peer operations only make sense when done implicitly.

I do disagree with this, with the obvious example being during an update to a new application version - there may be several of these operations (plus other stuff, like schema updates etc) and if any one of those operations fails, they should all fail so that the update is not partially applied.

I will not push back against the both of you on this, but I do think explicit transactions here are useful.

Fred and I spoke about this last week, currently (in develop), the P2P interface only looks like it supports explicit transactions - it doesn't actually respect them. This PR fixes this by making the interface reflect what is actually supported.

I'm happy with the change, it is better than what was before :)

AndrewSisley · 2023-10-04T19:39:52Z

I'm happy to pull those changes into a new PR if you'd prefer.

I would prefer it, it is a big change, and should be in the commit history. It also clutters up this PR a fair amount making review harder.

nasdf · 2023-10-04T19:51:25Z

I'm happy to pull those changes into a new PR if you'd prefer.

I would prefer it, it is a big change, and should be in the commit history. It also clutters up this PR a fair amount making review harder.

I'm working on this now and will update with a link when ready.

Update: added link to PR in description.

AndrewSisley

Looks good, but I still have some small questions.

AndrewSisley · 2023-10-10T15:10:55Z

cli/p2p_replicator_delete.go

 	var cmd = &cobra.Command{
-		Use:   "delete <peer>",
+		Use:   "delete [-c, --collection] <peer>",
 		Short: "Delete a replicator. It will stop synchronizing",


todo: Please change the short/long wording to reflect this :)

AndrewSisley · 2023-10-13T18:33:18Z

README.md

@@ -244,57 +244,73 @@ When starting a node for the first time, a key pair is generated and stored in i

 Each node has a unique `PeerID` generated from its public key. This ID allows other nodes to connect to it.

+To view your node's peer info:


praise: Thank you for adding this!

AndrewSisley · 2023-10-13T18:36:57Z

README.md

+Set *nodeA* to actively replicate the Article collection to *nodeB*:
+
+```shell
+defradb client p2p replicator set -c Article <peer_info_of_nodeB>


suggestion: I had to check the help text of defradb client p2p replicator set to be sure that <peer_info_of_nodeB> meant the full json object - it might be worth renaming this to reflect that.

I updated it to nodeB_peer_info_json to hopefully make it more obvious.

AndrewSisley · 2023-10-13T18:44:06Z

db/db.go

-func (db *db) Close(ctx context.Context) {
-	log.Info(ctx, "Closing DefraDB process...")
+func (db *db) Close() {
+	log.Info(context.Background(), "Closing DefraDB process...")


question: Was this what prompted the attempted removal of the ctx from the log interface?

thought: This does feel incorrect, as context has been defined outside of this function, and whilst at the moment as far as I can see they are all untouched context.Background()s, the context is not owned by this function, and could be modified by those owning layers, at which point this function would become not just conceptually incorrect, but tangibly so too. It might be worth chatting about this - whilst a minor/unimportant part of the codebase, this kind of feels like a drop in standards and as such I was temped to mark this as a todo.

Yeah this was the exact area where I ran into the log context. I plan on cleaning this up in the future when switching to the new log/slog go package.

Okay, I'm happy enough then if it is left as-is in the short term then if others are :)

AndrewSisley · 2023-10-13T18:47:58Z

tests/clients/cli/wrapper.go

+		return peer.AddrInfo{}
+	}
+	var info peer.AddrInfo
+	if err := json.Unmarshal(data, &info); err != nil {


question: Here, and in the if above, we are ignoring the error. Other functions in this wrapper seem to return their errors. How confident are you on the safety of this? Could it not lead to the tests missing panics and errors that because of this only appear in the logs?

I could change the API signature to return an error here. My thought process was that an empty peer.AddrInfo struct represents an error and any function using that empty address should return an error.

It feels a bit wrong to change the production interface for the sake of a test wrapper (even if just to add an error return param). Maybe we could just panic here for now (instead of returning empty)?

AndrewSisley · 2023-10-13T18:55:49Z

tests/integration/setup.go

@@ -0,0 +1,256 @@
+// Copyright 2023 Democratized Data Foundation


praise: I like that this is being broken out of the main (and large) utils2.go file

suggestion: I think it might be worth splitting it up again, I do dislike 'setup' files in general, as they often reduce the cohesion of the codebase, moving stuff away from where it is used. However here we only really have two things, and they are independent. We could split this file up into clients.go and db.go (or similar names), this would keep the database stuff together, and the client stuff together, both out of the main utils.go, and avoid us creating a general purpose, lets shove everything in it, setup file that risks growing horribly in the future if/when we add more stuff that requires setup.

There is also an argument to be had that 'setup' here is misleading (particularly considering the large init func at the top of the file), as it doesn't actually setup up the database or clients, it hosts the functions that the utils2.go calls to set them up.

I like that idea. I split them into db.go and client.go and put the init back into utils2.go

I think you forgot to add+push the new files :)

and put the init back into utils2.go

Why did you chose to return the init? I did picture that, and the declarations, being split up too (execution order didn't look like it mattered here).

Okay I see what you mean. I split the init logic but left the mutation variables in utils since they are not db or client specific.

commit c8bde64 Author: Islam Aliev <[email protected]> Date: Sat Oct 14 00:26:22 2023 +0200 feat: Make queries utilise secondary indexes (sourcenetwork#1925) ## Relevant issue(s) Resolves sourcenetwork#1555 ## Description With this change the the secondary indexes are utilised during querying data. A dedicated `Indexer` fetcher is implemented to perform fetching of values of indexed fields. Now there is a separate `filter` package that houses a lot of methods for working with filters. A new metric `indexesFetched` is introduced into `@explain` to provide information about how many indexes has been fetched. It also includes an update to the testing framework to allow adding custom asserters. The new ExplainResultsAsserter is used with this new feature.

AndrewSisley

LGTM, thanks Keenan :)

## Relevant issue(s) Resolves sourcenetwork#1883 ## Description Blocked by sourcenetwork#1927 This PR moves the `client.P2P` implementation from `client.DB` to `net.Node`. This fixes the problems mentioned in the issue above and should increase test coverage of the HTTP and CLI clients. ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] 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](tools/configs/chglog/config.yml)). - [x] 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? `make test` Specify the platform(s) on which this was tested: - MacOS

nasdf added 13 commits October 2, 2023 12:22

fix typo

022af8f

add client.P2P interface stubs to net.Peer

a49626a

remove P2P interface functions from db. implement P2P interface funct…

346fbce

…ions in net/peer

net package tests passing

55e2f21

remove grpc config

1c3f4e1

split p2p interface in cli and http. fix failing tests

175c5a8

clean up http handler constructor func

292150c

update p2p interface with test methods temporarily

13f2fff

move integration test setup to new file

1cfdda3

fix nil cancel in node close

c55ea7e

integration tests passing

1ec5f38

fix linter errors

e5b7b53

implement PeerInfo method in cli wrapper

eb04aed

nasdf self-assigned this Oct 3, 2023

nasdf added area/api Related to the external API component refactor This issue specific to or requires *notable* refactoring of existing codebases and components area/p2p Related to the p2p networking system labels Oct 3, 2023

nasdf added this to the DefraDB v0.8 milestone Oct 3, 2023

nasdf added 5 commits October 3, 2023 13:15

fix incorrect context key usage in cli

ab11b65

restore node addresses on restart in tests

78ac2cb

fix node restart address logic in tests

f3887a4

move test functions from p2p interface to tests/clients. disable p2p …

5c2d60b

…relay in tests

fix race condition in test

2a01990

nasdf and others added 3 commits October 4, 2023 10:24

add replicator delete test

7e6bb4f

remove flaky logging asserts from net package

94a7b8b

Merge branch 'develop' into nasdf/refactor/p2p-client-interface

592e411

nasdf marked this pull request as ready for review October 4, 2023 19:11

nasdf requested a review from a team October 4, 2023 19:11

AndrewSisley requested changes Oct 4, 2023

View reviewed changes

nasdf added 2 commits October 10, 2023 16:26

Merge branch 'develop' into nasdf/refactor/p2p-client-interface

687f0e2

restore peer subscribe tests

fa6b67e

nasdf requested a review from AndrewSisley October 11, 2023 00:33

nasdf added 4 commits October 11, 2023 08:55

add replicator cli examples

8cca432

update readme p2p examples to reflect changes

4be14e7

fix incorrect readme cli command

edb9249

remove grpc flag from readme

b907f30

nasdf requested a review from a team October 12, 2023 17:19

AndrewSisley reviewed Oct 13, 2023

View reviewed changes

nasdf and others added 8 commits October 13, 2023 12:50

improve cli replicator docs

25139aa

split tests setup into db and client files

580f016

fix replicator example in readme

4532bfd

split test init logic into client and db files

6ec4577

panic if cli test wrapper peer info fails

76c9b5f

Merge branch 'develop' into nasdf/refactor/p2p-client-interface

a96746c

Merge branch 'develop' into nasdf/refactor/p2p-client-interface

b967436

nasdf requested a review from AndrewSisley October 16, 2023 15:38

AndrewSisley approved these changes Oct 16, 2023

View reviewed changes

nasdf added 2 commits October 16, 2023 14:45

possible fix for node restart race condition in tests

abf2af3

Merge branch 'develop' into nasdf/refactor/p2p-client-interface

40b2603

nasdf merged commit 25d4767 into sourcenetwork:develop Oct 16, 2023
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: P2P client interface #1924

refactor: P2P client interface #1924

nasdf commented Oct 3, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading

AndrewSisley commented Oct 4, 2023

nasdf commented Oct 4, 2023

AndrewSisley left a comment

AndrewSisley Oct 4, 2023

AndrewSisley Oct 4, 2023

nasdf Oct 4, 2023

AndrewSisley Oct 10, 2023

AndrewSisley Oct 4, 2023

nasdf Oct 4, 2023

fredcarle Oct 5, 2023

AndrewSisley Oct 5, 2023

AndrewSisley Oct 10, 2023

AndrewSisley commented Oct 4, 2023

nasdf commented Oct 4, 2023 •

edited

Loading

AndrewSisley left a comment

AndrewSisley Oct 10, 2023

AndrewSisley Oct 13, 2023

AndrewSisley Oct 13, 2023

nasdf Oct 13, 2023

AndrewSisley Oct 13, 2023

nasdf Oct 13, 2023

AndrewSisley Oct 13, 2023 •

edited

Loading

AndrewSisley Oct 13, 2023

nasdf Oct 13, 2023

AndrewSisley Oct 13, 2023

AndrewSisley Oct 13, 2023

nasdf Oct 13, 2023

AndrewSisley Oct 13, 2023 •

edited

Loading

nasdf Oct 13, 2023

AndrewSisley left a comment

		@@ -350,79 +349,38 @@ func (apicfg *APIConfig) AddressToURL() string {

		// NetConfig configures aspects of network and peer-to-peer.
		type NetConfig struct {

		@@ -244,57 +244,73 @@ When starting a node for the first time, a key pair is generated and stored in i

		Each node has a unique `PeerID` generated from its public key. This ID allows other nodes to connect to it.

		To view your node's peer info:

		@@ -0,0 +1,256 @@
		// Copyright 2023 Democratized Data Foundation

refactor: P2P client interface #1924

refactor: P2P client interface #1924

Conversation

nasdf commented Oct 3, 2023 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

AndrewSisley commented Oct 4, 2023

nasdf commented Oct 4, 2023

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

AndrewSisley commented Oct 4, 2023

nasdf commented Oct 4, 2023 • edited Loading

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

AndrewSisley Oct 13, 2023 • 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

AndrewSisley Oct 13, 2023 • edited Loading

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

nasdf commented Oct 3, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading

nasdf commented Oct 4, 2023 •

edited

Loading

AndrewSisley Oct 13, 2023 •

edited

Loading

AndrewSisley Oct 13, 2023 •

edited

Loading