fix: Improve P2P event handling #1388

fredcarle · 2023-04-21T18:00:15Z

Relevant issue(s)

Resolves #1334
Resolves #1335
Resolves #1336
Resolves #1337
Resolves #1338
Resolves #1375

Description

This PR seems to have fixed all known flakyness from the P2P integration tests. It does so at different levels:

We prevent pre peer connection and pre replicator configuration actions from having their events received by the wait function by adding a 100ms sleep call pre setup.
We ensure that the wait go routines are ready before continuing with the actions.
We add a docQueue to the PushLog function so that we don't handle multiple updates to the same document at once. This helps avoid unnecessary transaction conflicts. This would also sometimes cause the PushLog function to hang and the RPC call hitting a timeout.
We handle pubsub topic subscriptions a little better by subscribing only to the collection OR the documents. Subscribing to documents and the collection would cause double the event handling.

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?

Integration test (a 6 hour run without errors)

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

Github CI
MacOS

AndrewSisley · 2023-04-21T18:04:18Z

tests/integration/p2p.go

@@ -140,6 +140,9 @@ func connectPeers(
 	nodes []*node.Node,
 	addresses []string,
 ) chan struct{} {
+	// If we have some database actions prior to connecting the peers, we want to ensure that they had time to
+	// complete before we connect. Otherwise we might wrongly catch them in our wait function.
+	time.Sleep(100 * time.Millisecond)


praise: Nice spot - I didn't notice this looking through your wip branch earlier :)

codecov · 2023-04-21T18:09:57Z

Codecov Report

Merging #1388 (93b74ce) into develop (8f853d2) will decrease coverage by 0.11%.
The diff coverage is 51.32%.

@@             Coverage Diff             @@
##           develop    #1388      +/-   ##
===========================================
- Coverage    70.76%   70.65%   -0.11%     
===========================================
  Files          185      185              
  Lines        17884    17934      +50     
===========================================
+ Hits         12655    12671      +16     
- Misses        4279     4305      +26     
- Partials       950      958       +8

Impacted Files	Coverage Δ
net/peer.go	`49.01% <26.00%> (-1.87%)`	⬇️
net/server.go	`64.06% <71.42%> (+0.51%)`	⬆️

... and 2 files with indirect coverage changes

AndrewSisley

Really great work Fred - thanks a whole bunch for sorting these out :)

Left a couple of small comments which would be good to resolve before merge.

AndrewSisley · 2023-04-21T18:08:28Z

net/peer.go

@@ -629,10 +627,16 @@ func (p *Peer) handleDocCreateLog(evt events.Update) error {
 		return errors.Wrap("failed to get DocKey from broadcast message", err)
 	}

+	// We need to register the document before pushing to the replicators if we want to
+	// ensure that we have subscribed to the topic.
+	err = p.RegisterNewDocument(p.ctx, dockey, evt.Cid, evt.Block, evt.SchemaID)


question: This change is not noted in the description, is it still needed, or is this left here accidentally?

I think it's still needed for the situation where the target would receive the push to replicator and then the target getting an update before the source registers to the topic. This probably would never happen in real life but it did happen in the tests.

AndrewSisley · 2023-04-21T18:11:09Z

net/peer.go

@@ -804,6 +808,30 @@ func (p *Peer) AddP2PCollections(collections []string) error {
 		addedTopics = append(addedTopics, col)
 	}

+	// If adding the collection topics succeeds, we remove the collections' documents
+	// from the pubsub topics to avoid receiving duplicate events.
+	for _, col := range collections {


thought: This is potentially very expensive - how long do you expect this code to live? It might be worth noting this in the public documentation.

At the moment I can't think of a better way to handle this. It would only be expensive if a dev decides to subscribe to a given collection after it has millions of documents. It's also a one-off event if someone was to do it so I don't think we need to worry about speed too much. Not at this time anyways.

I can add a warning to the doc though.

AndrewSisley · 2023-04-21T18:16:35Z

net/peer.go

+			return err
+		}
+		for key := range keyChan {
+			err := p.server.removePubSubTopic(key.Key.String())


suggestion: This will not be rolled back if the txn commit fails, but it should be. I do not see this as a new issue however (just expanded), as the addedTopics block also suffers from this. Strongly suggest creating a ticket to roll this back properly on commit error (might already be one, I'm not sure though).

Same issue exists in RemoveP2PCollections

Ticket opened 👍

AndrewSisley · 2023-04-21T18:19:30Z

net/server.go

+			if err != nil {
+				log.Info(ctx, "could not decode the peer id of the log creator", logging.NewKV("Error", err.Error()))
+			}
+			err = s.pushLogEmitter.Emit(EvtReceivedPushLog{


praise: I like that this is in a defer now

AndrewSisley · 2023-04-21T18:23:27Z

tests/integration/p2p.go

@@ -307,7 +317,7 @@ func configureReplicator(
 				docIDsSyncedToSource[currentdocID] = struct{}{}
 			}

-			if action.NodeID.HasValue() && action.NodeID.Value() == cfg.SourceNodeID {
+			if !action.NodeID.HasValue() || action.NodeID.Value() == cfg.SourceNodeID {


question: Are you sure this is correct?

If !action.NodeID.HasValue() then the document will be created synchronously and locally by the test framework in all nodes. Does the P2P system still transmit sync events if they already exist on the node?

todo: If this is correct, can you please add a comment documenting this :)

Does the P2P system still transmit sync events if they already exist on the node?

Yes. The Replicator doesn't know that the target has the document already so it still sends the created document which will cause the target to send a received push log event.

Description This PR seems to have fixed all known flakyness from the P2P integration tests. It does so at different levels: - We prevent pre peer connection and pre replicator configuration actions from having their events received by the wait function by adding a 100ms sleep call pre setup. - We ensure that the wait go routines are ready before continuing with the actions. - We add a docQueue to the PushLog function so that we don't handle multiple updates to the same document at once. This helps avoid unnecessary transaction conflicts. This would also sometimes cause the PushLog function to hang and the RPC call hitting a timeout. - We handle pubsub topic subscriptions a little better by subscribing only to the collection OR the documents. Subscribing to documents and the collection would cause double the event handling.

fredcarle added area/testing Related to any test or testing suite area/p2p Related to the p2p networking system action/no-benchmark Skips the action that runs the benchmark. labels Apr 21, 2023

fredcarle added this to the DefraDB v0.5.1 milestone Apr 21, 2023

fredcarle requested a review from a team April 21, 2023 18:00

fredcarle self-assigned this Apr 21, 2023

AndrewSisley reviewed Apr 21, 2023

View reviewed changes

AndrewSisley approved these changes Apr 21, 2023

View reviewed changes

fredcarle added 2 commits April 24, 2023 18:57

fix flaky p2p tests

d9b035a

apply feedback

93b74ce

fredcarle force-pushed the fredcarle/fix/p2p-create-order branch from 953bd35 to 93b74ce Compare April 24, 2023 22:57

fredcarle merged commit 322807f into develop Apr 24, 2023

fredcarle deleted the fredcarle/fix/p2p-create-order branch April 24, 2023 23:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Improve P2P event handling #1388

fix: Improve P2P event handling #1388

fredcarle commented Apr 21, 2023 •

edited

Loading

AndrewSisley Apr 21, 2023

codecov bot commented Apr 21, 2023 •

edited

Loading

AndrewSisley left a comment

AndrewSisley Apr 21, 2023

fredcarle Apr 21, 2023

AndrewSisley Apr 21, 2023

fredcarle Apr 21, 2023

AndrewSisley Apr 21, 2023

fredcarle Apr 21, 2023

AndrewSisley Apr 21, 2023

AndrewSisley Apr 21, 2023

AndrewSisley Apr 21, 2023

fredcarle Apr 22, 2023

fix: Improve P2P event handling #1388

fix: Improve P2P event handling #1388

Conversation

fredcarle commented Apr 21, 2023 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

Choose a reason for hiding this comment

codecov bot commented Apr 21, 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

fredcarle commented Apr 21, 2023 •

edited

Loading

codecov bot commented Apr 21, 2023 •

edited

Loading