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

CBG-4067: do not release doc sequence upon timeout error #7016

Merged
merged 5 commits into from
Jul 29, 2024
Merged

Conversation

gregns1
Copy link
Contributor

@gregns1 gregns1 commented Jul 26, 2024

CBG-4067

  • Added check to only release doc sequences when a write error is not a timeout error.
  • Added some functionality to leaky bucket to force return a timeout error upon a successful write (open to ideas of better forcing a timeout here)
  • Test also covers us correctly releasing a doc sequence upon a different et error at write time by casuing conflict uszing leaky bucket functionality

Pre-review checklist

  • Removed debug logging (fmt.Print, log.Print, ...)
  • Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
  • Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Integration Tests

@gregns1 gregns1 self-assigned this Jul 26, 2024
@gregns1 gregns1 assigned torcolvin and unassigned gregns1 Jul 26, 2024
Copy link
Collaborator

@torcolvin torcolvin left a comment

Choose a reason for hiding this comment

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

This LGTM, just some questions about the tests, and some code nits you can take or leave.

@@ -121,6 +121,8 @@ type LeakyBucketConfig struct {

ForceErrorSetRawKeys []string // Issuing a SetRaw call with a specified key will return an error

ForceTimeoutErrorOnUpdateKeys []string // Specified keys will return timeout error AFTER write is sent to server
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is interesting because this definitely means the write occurred. However, in the case of this ticket, we don't know whether the write went through or not? Is there a reason to actually have this error after it writes and not before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want this behaviour as this is what the customer saw. See ticket linked to this for more info but they had write, server under load and operation times out, but write eventually succeeds but we've timeout and release the document sequence. Then then the successful write is ignored at cache due to duplicate sequence with the released sequence.

It certainly seems odd to do it this way but its the only way I could think of mocking/forcing this situation reliably in a test. We cannot be sure when we hit timeout that the write will eventually go through or not but the changes this PR makes will protect against docs being missed in replications if the write does succeed.

@gregns1 gregns1 requested a review from torcolvin July 29, 2024 11:01
@torcolvin torcolvin merged commit 9026ed8 into main Jul 29, 2024
38 checks passed
@torcolvin torcolvin deleted the CBG-4067 branch July 29, 2024 14:53
adamcfraser added a commit that referenced this pull request Sep 27, 2024
…rror (#7133)

* CBG-4067: do not release doc sequence upon timeout error (#7016)

* CBG-4067 Only skip releasing sequences for timeouts (#7050)

We want to release sequences in scenarios where we know the write did not occur.

---------

Co-authored-by: Gregory Newman-Smith <[email protected]>
Co-authored-by: Adam Fraser <[email protected]>
bbrks added a commit that referenced this pull request Oct 8, 2024
…rror (#7133)

* CBG-4067: do not release doc sequence upon timeout error (#7016)

* CBG-4067 Only skip releasing sequences for timeouts (#7050)

We want to release sequences in scenarios where we know the write did not occur.

---------

Co-authored-by: Gregory Newman-Smith <[email protected]>
Co-authored-by: Adam Fraser <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants