Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Retry test doc mutation on transaction conflict #1366

Merged
merged 4 commits into from
Apr 18, 2023

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #1365

Description

Adds Retry mechanics for test doc create and delete on transaction conflict.

The logic existed for update, and has now been expanded to cover Create and Update.

It could probably be extended to cover requests, but I dont think any P2P tests use mutation requests at the moment.

@AndrewSisley AndrewSisley added area/testing Related to any test or testing suite action/no-benchmark Skips the action that runs the benchmark. labels Apr 17, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.5.1 milestone Apr 17, 2023
@AndrewSisley AndrewSisley requested a review from a team April 17, 2023 22:51
@AndrewSisley AndrewSisley self-assigned this Apr 17, 2023
@AndrewSisley AndrewSisley changed the title test: Retry test doc mutation on transaction conflict test: Retry test doc mutation on transaction conflict Apr 17, 2023
@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #1366 (2cc440b) into develop (291b2b1) will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1366      +/-   ##
===========================================
- Coverage    70.59%   70.55%   -0.05%     
===========================================
  Files          184      184              
  Lines        17835    17835              
===========================================
- Hits         12591    12583       -8     
- Misses        4288     4295       +7     
- Partials       956      957       +1     

see 3 files with indirect coverage changes

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving but I have a couple suggestions before you merge :)

// about this in our tests so we just retry a few times until it works (or the
// retry limit is breached - important incase this is a different error)
func withRetry(
ctx context.Context,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: ctx isn't used so you can probably remove it from the parameters.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Apr 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ops cheers - it was used, and then I switched to the func param. Will remove

  • rm ctx

Comment on lines +865 to +866
nodes []*node.Node,
nodeID int,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: You could use node *node.Node instead and avoid having to pass 2 params.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Apr 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was 50-50 about that one, in my head at least a lot of the test utils code does stuff like that - preferring the refactoring out of the finding of the correct thing instead of saving on param count. Will have a look in a bit and then see what I feel like in the moment unless you have a strong preference.

  • maybe just pass in node

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving as-is

@AndrewSisley AndrewSisley merged commit eb3a0a8 into develop Apr 18, 2023
@AndrewSisley AndrewSisley deleted the sisley/test/I1365-create-retry branch April 18, 2023 12:59
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…1366)

* Refactor update-save retry logic

* Retry on create doc transaction conflict

* Retry on delete doc transaction conflict
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/testing Related to any test or testing suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test utils CreateDoc action does not retry on txn conflict
2 participants