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-4303: conflicting writes muti actor tests, skipping failures #7205

Merged
merged 5 commits into from
Dec 2, 2024

Conversation

gregns1
Copy link
Contributor

@gregns1 gregns1 commented Nov 22, 2024

CBG-4303

  • Adds test for multi actor conflicting writes
  • Skipping all tests due to general flake issues and panics against rosmar. tickets are CBG-4379 and CBG-4378 respectively

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

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.

LGTM in concept, lots of readability comments which you can think about and take or leave.

Speaking of readability, do you think it would make sense to put the different test scenarios (single actor, multi actor non conflicting, multi actor conflicting) into their own files? I can see these getting out of hand but perhaps there aren't enough tests yet to warrant that.

topologytest/couchbase_server_peer_test.go Outdated Show resolved Hide resolved
topologytest/hlv_test.go Outdated Show resolved Hide resolved
topologytest/hlv_test.go Outdated Show resolved Hide resolved
topologytest/hlv_test.go Outdated Show resolved Hide resolved
topologytest/hlv_test.go Outdated Show resolved Hide resolved
t.Run(tc.description(), func(t *testing.T) {
peers, replications := setupTests(t, tc.topology, "")

stopPeerReplications(t, replications)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to stop replications before creating conflicting docs? Is this because CreateDocument can fail?

I think this is a good idea to debug tests at first but I think we'll want these writes to occur eventually while peers are active. I think this is worth marking as such and filing a followup ticket.

I think it might improve readability if we provide something like options to setupTests to say startReplications and activePeer. I am a big fan of setupTestsOptions structs so it's easy to understand what arguments like bools and strings mean.

Actually I think activePeer might be now unused...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I'm stopping replications as the spec says to do so to create conflicting mutations. I think it makes sense at this stage to ensure we don't have timing issues etc. But I do agree we should have some concurrent updates but that maybe should be separate ticket.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely a separate ticket!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed CBG-4380 for it!

topologytest/hlv_test.go Outdated Show resolved Hide resolved
topologytest/hlv_test.go Show resolved Hide resolved
@gregns1 gregns1 assigned torcolvin and unassigned gregns1 Dec 2, 2024
@torcolvin torcolvin merged commit 5196d8a into release/anemone Dec 2, 2024
36 checks passed
@torcolvin torcolvin deleted the CBG-4303 branch December 2, 2024 14:54
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