fix: Improve DAG sync with highly concurrent updates #1031

fredcarle · 2023-01-13T02:33:08Z

Relevant issue(s)

Resolves #1029
Resolves #1030

Description

This PR resolves the eventual consistency bug when highly concurrent updates would cause the DAG syncer to fail reaching consistency. It also adds a simple retry mechanic to avoid sync issues when there is a transaction conflict on the head store.

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-01-13T02:37:45Z

Codecov Report

Merging #1031 (a856805) into develop (cb36b2d) will increase coverage by 0.02%.
The diff coverage is 61.84%.

@@             Coverage Diff             @@
##           develop    #1031      +/-   ##
===========================================
+ Coverage    57.79%   57.82%   +0.02%     
===========================================
  Files          173      173              
  Lines        19462    19495      +33     
===========================================
+ Hits         11249    11273      +24     
- Misses        7223     7232       +9     
  Partials       990      990

Impacted Files	Coverage Δ
client/errors.go	`60.00% <0.00%> (-4.29%)`	⬇️
config/configfile.go	`75.00% <ø> (ø)`
core/crdt/lwwreg.go	`68.96% <40.00%> (-5.43%)`	⬇️
net/process.go	`74.62% <63.26%> (-0.97%)`	⬇️
cli/start.go	`44.04% <66.66%> (+0.75%)`	⬆️
db/db.go	`72.47% <66.66%> (-0.53%)`	⬇️
config/config.go	`75.58% <100.00%> (+0.07%)`	⬆️
core/crdt/base.go	`73.07% <100.00%> (ø)`
net/pb/net.pb.go	`15.49% <0.00%> (-0.17%)`	⬇️
... and 3 more

jsimnz

A single pretty basic item is needed to change. Made a quick note aswell.

jsimnz · 2023-01-13T02:43:49Z

core/crdt/lwwreg.go

+		// Do not use the first byte of the current value in the comparison.
+		// It's metadata that will falsify the result.
+		if len(curValue) > 0 {
+			curValue = curValue[1:]
+		}


This isn't exceptionally important. It can stay since technically its more accurate, but it wouldn't change the comparison either way, since its s a byte of metadata that corresponds to the CRDT type, which only get compared within the same type, so it does technically canel itself out.

But yes, technically this is more accurate, just wanted to add some insight.

It actually fails in Andy's PR if this is not done.

ohhh, yes you are right. I see. Thanks for the clarification.

I was going to ask @fredcarle have you tested this using the tests in my PR? How do you know this PR makes a tangible improvement?

I have yes. With a couple minor changes. Everything passes as it should.

Nice that's awesome - cheers Fred

jsimnz · 2023-01-13T02:45:49Z

net/process.go

+	// TODO: Implement better transaction retry mechanics
+	// Github issue #1028
+	for {


suggestion: In the short term, can we apply a (perhaps configurable/option) limit to the number of retry attempts, instead of "infinite". Its unlikely to spin forever trying and constantly failing to commit, but just for santy / cleanlyness

Absolutely. Will do.

shahzadlone

Minor suggestion.

shahzadlone · 2023-01-13T13:33:41Z

net/process.go

+			logging.NewKV("DocKey", dockey),
+			logging.NewKV("CID", c),
+		)
+		height := delta.GetPriority()


todo: Move this below line 89 or do the suggestion on line 90.

This is existing code that was simply moved inside a for loop. Do really want me to make the change even if it's unrelated to the the fix? Same would apply for your suggestion bellow.

Can do it in a separate commit, you are touching this part of the code anyways and is a one liner.

This isn't worth making an issue for and I doubt we will remember to improve this.

Sounds good. I just wanted to know your opinion on that situation :)

shahzadlone · 2023-01-13T13:35:01Z

net/process.go

-	)
-	height := delta.GetPriority()
+		ng := p.createNodeGetter(crdt, getter)
+		cids, err := crdt.Clock().ProcessNode(ctx, ng, c, height, delta, nd)


suggestion:

Suggested change

cids, err := crdt.Clock().ProcessNode(ctx, ng, c, height, delta, nd)

cids, err := crdt.Clock().ProcessNode(ctx, ng, c, delta.GetPriority(), delta, nd)

AndrewSisley

Thanks Fred, looks good to me so long as there is some protection added against the infinite loop that John flagged :)

jsimnz

LGTM. Minor suggestions and nitpicks, can change/merge as you see fit!

db/db.go

net/process.go

cli/start.go

config/config.go

client/db.go

Change applied

Relevant issue(s) Resolves #1029 Resolves #1030 Description This PR resolves the eventual consistency bug when highly concurrent updates would cause the DAG syncer to fail reaching consistency. It also adds a simple retry mechanic to avoid sync issues when there is a transaction conflict on the head store.

) Relevant issue(s) Resolves sourcenetwork#1029 Resolves sourcenetwork#1030 Description This PR resolves the eventual consistency bug when highly concurrent updates would cause the DAG syncer to fail reaching consistency. It also adds a simple retry mechanic to avoid sync issues when there is a transaction conflict on the head store.

fredcarle added bug Something isn't working area/crdt Related to the (Merkle) CRDT system area/p2p Related to the p2p networking system action/no-benchmark Skips the action that runs the benchmark. labels Jan 13, 2023

fredcarle added this to the DefraDB v0.5 milestone Jan 13, 2023

fredcarle requested a review from a team January 13, 2023 02:33

fredcarle self-assigned this Jan 13, 2023

fredcarle changed the title ~~fix: Improve eventual consistency with highly concurrent updates~~ fix: Improve DAG sync with highly concurrent updates Jan 13, 2023

jsimnz requested changes Jan 13, 2023

View reviewed changes

shahzadlone previously requested changes Jan 13, 2023

View reviewed changes

AndrewSisley approved these changes Jan 13, 2023

View reviewed changes

This was referenced Jan 16, 2023

fix: Rework the P2P integration tests #989

Merged

Change detector tests do not assert that priority is consistent across db versions #1036

Open

fredcarle added 4 commits January 16, 2023 18:17

fix eventual consitency issue

ef8ed33

add documentation

a532256

change test to work with behavior change

fa8fc9b

add maxRetries to db config

c949565

fredcarle force-pushed the fredcarle/fix/I1029-register-priority-fix branch from e129f6e to c949565 Compare January 16, 2023 23:17

fredcarle requested a review from jsimnz January 16, 2023 23:18

remove unused field

7cf1348

jsimnz approved these changes Jan 16, 2023

View reviewed changes

db/db.go Outdated Show resolved Hide resolved

net/process.go Outdated Show resolved Hide resolved

cli/start.go Outdated Show resolved Hide resolved

config/config.go Outdated Show resolved Hide resolved

client/db.go Outdated Show resolved Hide resolved

apply feedback

a856805

fredcarle force-pushed the fredcarle/fix/I1029-register-priority-fix branch from f92fc30 to a856805 Compare January 17, 2023 00:35

fredcarle merged commit 210423c into develop Jan 17, 2023

fredcarle deleted the fredcarle/fix/I1029-register-priority-fix branch January 17, 2023 01:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Improve DAG sync with highly concurrent updates #1031

fix: Improve DAG sync with highly concurrent updates #1031

fredcarle commented Jan 13, 2023

codecov bot commented Jan 13, 2023 •

edited

Loading

jsimnz left a comment

jsimnz Jan 13, 2023

fredcarle Jan 13, 2023

jsimnz Jan 13, 2023

AndrewSisley Jan 13, 2023

fredcarle Jan 13, 2023

AndrewSisley Jan 13, 2023

jsimnz Jan 13, 2023

fredcarle Jan 13, 2023

shahzadlone left a comment

shahzadlone Jan 13, 2023

fredcarle Jan 13, 2023

shahzadlone Jan 13, 2023

fredcarle Jan 13, 2023

shahzadlone Jan 13, 2023

AndrewSisley left a comment

jsimnz left a comment

	cids, err := crdt.Clock().ProcessNode(ctx, ng, c, height, delta, nd)
	cids, err := crdt.Clock().ProcessNode(ctx, ng, c, delta.GetPriority(), delta, nd)

fix: Improve DAG sync with highly concurrent updates #1031

fix: Improve DAG sync with highly concurrent updates #1031

Conversation

fredcarle commented Jan 13, 2023

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Jan 13, 2023 • edited Loading

Codecov Report

jsimnz 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

shahzadlone 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

AndrewSisley left a comment

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 13, 2023 •

edited

Loading