refactor: Remove net GRPC API #1927

nasdf · 2023-10-04T20:53:31Z

Relevant issue(s)

N/A

Description

This PR removes the GRPC API from the net package. The HTTP and CLI interfaces now include this functionality.

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

codecov · 2023-10-04T21:13:59Z

Codecov Report

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

Comparison is base (ce7d778) 74.60% compared to head (8b16e2c) 74.62%.

@@             Coverage Diff             @@
##           develop    #1927      +/-   ##
===========================================
+ Coverage    74.60%   74.62%   +0.02%     
===========================================
  Files          234      234              
  Lines        23116    23044      -72     
===========================================
- Hits         17244    17195      -49     
+ Misses        4696     4671      -25     
- Partials      1176     1178       +2

Flag	Coverage Δ
all-tests	`74.62% <68.05%> (+0.02%)`	⬆️

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_remove.go	`100.00% <100.00%> (ø)`
config/config.go	`73.41% <100.00%> (+1.01%)`	⬆️
http/handler.go	`96.81% <100.00%> (ø)`
net/config.go	`94.83% <ø> (-0.89%)`	⬇️
net/node.go	`87.90% <100.00%> (-3.90%)`	⬇️
cli/start.go	`25.51% <0.00%> (+2.01%)`	⬆️
http/client.go	`47.70% <57.14%> (-0.53%)`	⬇️
http/handler_store.go	`55.79% <33.33%> (-1.30%)`	⬇️
db/txn_db.go	`57.89% <53.85%> (+0.45%)`	⬆️
... and 1 more

... and 10 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 ce7d778...8b16e2c. Read the comment docs.

fredcarle · 2023-10-05T00:23:13Z

net/node_test.go

question: I've never seen these test being flaky. I'm not sure the tests are relevant anymore if we don't check the outcome. What do you think caused them to become flaky?

When it failed it was logging an extra line. My guess is that it was always prone to being flaky. Now that we run the parallel test matrix it just appears more often.

net/node.go

AndrewSisley · 2023-10-05T13:32:00Z

tests/integration/net/state/simple/replicator/with_create_update_test.go

 			testUtils.CreateDoc{
 				// This document is created in all nodes
 				Doc: `{
 					"Name": "John",
 					"Age": 21
 				}`,
 			},
+			testUtils.ConfigureReplicator{


todo: This is changing what the test tests, and if there is flakiness here, it should probably be handled with a WaitForSync action instead of changing the test my moving when configuration happens.

That said, where have you seen this flakiness? Do you have an issue or an error log?

The CLI tests fail with the original test setup. If the replicator syncs the document before the second node attempts to create it, then it will fail. I will swap it for a WaitForSync like you suggested.

But only the CLI tests failed/were flaky? That suggests a problem with the production code, not the test.

If the other mutation types are not flaky, and the CLI is, the CLI is not behaving the same way as the others.

I believe this is due to the CLI being the slowest implementation to test. If you look through the original test steps you can see where the race condition happens. The document syncs from node 0 to node 1 before node 1 has the chance to create it.

I believe this is due to the CLI being the slowest implementation to test.

That does not make sense to me though, as the slowness is outside of the execution bounds in which speed matters (RE race), and if anything the CLI should be more resilient to flakiness here, as there is a larger gap between submitting the Create, and the Update.

Where x = main test thread, y = create async stuff, z = update async stuff, * = mutation type specific x stuff:

x * // Create doc action start x * x y // x y x y x y x y // Create doc action end x * // Update doc action start x * x z x z x z

When running CLI tests * may occupy more time, but that should only have the same effect as adding in a wait/sleep, and should reduce the risk of hitting a race condition (where y and z overlap).

From memory I also believe that the ConfigureReplicator action includes a wait anyway.

TestP2PPeerReplicatorWithUpdate also seems to run fine with the same create-config-update flow?

Discussed over discord. I misunderstood the nature of the failure, it makes sense to me now, and am happy with this test being changed here. This problem likely affects a bunch of other tests though, and that work should be done in another issue/PR (not here).

I created an issue to start tracking tests with potential race condition #1935

AndrewSisley · 2023-10-05T13:41:00Z

tests/integration/p2p.go

 	//
 	// A [NonExistentCollectionID] may be provided to test non-existent collection IDs.
-	CollectionIDs []int
+	CollectionID int


praise: This is a good change, thanks - the test action should reflect the structure of the production operation it represents. AddP2PCollection takes a single id, so so should the action IMO.

thought (don't bother now): This is however an inedpendantly useful change, that is not just a one-liner, and is independent from the goal of the PR. It should have been in another PR as far as I can see.

I'm not sure which direction would be better. Remove the option of adding multiple collections in one function call or adding the option to add multiple collections at a time via the CLI/HTTP API. The latter is what I had intended when creating the functionality but the CLI had been implemented with the limitation of one collection at a time (I think I missed that in review).

I'm happy to consider either option as long as it matches the client API.

I'll let you decide which one you think creates a better user experience.

defradb client p2pcollection add Users defradb client p2pcollection add Address

vs

defradb client p2pcollection add Users,Address

I've updated it to support multiple collections like we discussed.

AndrewSisley · 2023-10-05T13:45:41Z

tests/integration/net/state/simple/peer/subscribe/with_add_test.go

-	testUtils.ExecuteTestCase(t, test)
-}
-
-func TestP2PSubscribeAddValidThenErroneousCollectionID(t *testing.T) {


question: Why have these tests been removed?

TestP2PSubscribeAddSingleErroneousCollectionID tests the same thing since only one subscription at a time is allowed.

TestP2PSubscribeAddSingleErroneousCollectionID does not test that other existing collection-peer subscriptions are unaffected by the attempt to configure a bad collection ID. This test does.

My bad, I had misunderstood these tests. I restored them with one minor modification to make it pass.

AndrewSisley · 2023-10-05T17:17:24Z

tests/integration/net/state/simple/peer/subscribe/with_add_test.go

@@ -337,10 +341,6 @@ func TestP2PSubscribeAddNone(t *testing.T) {
 				SourceNodeID: 1,
 				TargetNodeID: 0,
 			},
-			testUtils.SubscribeToCollection{


todo: Either the name of this test should be changed slightly, or the test deleted (if this is tested elsewhere, which it might be).

AndrewSisley

LGTM, approving now, assuming Fred's comments are addressed sensibly, as well as/including the log test stuff (maybe split that out to another PR and merge this stuff now?). Plus the one new minor todo from me :)

Thanks Keenan :)

## 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 - [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

## Relevant issue(s) N/A ## Description This PR removes the GRPC API from the net package. The HTTP and CLI interfaces now include this functionality. ## 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

## 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 self-assigned this Oct 4, 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 labels Oct 4, 2023

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

nasdf requested a review from a team October 4, 2023 20:54

nasdf mentioned this pull request Oct 4, 2023

refactor: P2P client interface #1924

Merged

4 tasks

fredcarle reviewed Oct 5, 2023

View reviewed changes

net/node.go Show resolved Hide resolved

AndrewSisley reviewed Oct 5, 2023

View reviewed changes

AndrewSisley approved these changes Oct 5, 2023

View reviewed changes

nasdf requested a review from fredcarle October 5, 2023 23:39

nasdf added 11 commits October 10, 2023 09:47

remove net grpc api

efb245e

remove flaky log tests from net package

2c53ac6

fix flaky replicator test

46ea482

fix linter error

aa3ba75

restore removed tests

463b641

fix typo

e381088

update test name to better match test case

e4b4437

remove redundant test case

a856477

add support for adding or deleting multiple p2p collections

d192a75

revert test changes

24d7721

fix for cli adding or removing empty p2p collection.

c4d386c

nasdf force-pushed the nasdf/refactor/remove-net-grpc-api branch from 9220ac4 to c4d386c Compare October 10, 2023 16:52

remove redundant node tests

8b16e2c

nasdf merged commit bc4c704 into sourcenetwork:develop Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Remove net GRPC API #1927

refactor: Remove net GRPC API #1927

nasdf commented Oct 4, 2023

codecov bot commented Oct 4, 2023 •

edited

Loading

fredcarle Oct 5, 2023

nasdf Oct 5, 2023

AndrewSisley Oct 5, 2023 •

edited

Loading

nasdf Oct 5, 2023

AndrewSisley Oct 5, 2023

nasdf Oct 5, 2023 •

edited

Loading

AndrewSisley Oct 5, 2023

AndrewSisley Oct 5, 2023

nasdf Oct 5, 2023

AndrewSisley Oct 5, 2023 •

edited

Loading

fredcarle Oct 5, 2023

nasdf Oct 5, 2023

fredcarle Oct 5, 2023

nasdf Oct 5, 2023

AndrewSisley Oct 5, 2023

nasdf Oct 5, 2023

AndrewSisley Oct 5, 2023 •

edited

Loading

nasdf Oct 5, 2023

AndrewSisley Oct 5, 2023

AndrewSisley left a comment

refactor: Remove net GRPC API #1927

refactor: Remove net GRPC API #1927

Conversation

nasdf commented Oct 4, 2023

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasdf Oct 5, 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

AndrewSisley Oct 5, 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

Choose a reason for hiding this comment

AndrewSisley Oct 5, 2023 • edited Loading

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

edited

Loading

AndrewSisley Oct 5, 2023 •

edited

Loading

nasdf Oct 5, 2023 •

edited

Loading

AndrewSisley Oct 5, 2023 •

edited

Loading

AndrewSisley Oct 5, 2023 •

edited

Loading