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-4289 fix import CV value for HLV code #7146

Open
wants to merge 3 commits into
base: release/anemone
Choose a base branch
from

Conversation

torcolvin
Copy link
Collaborator

@torcolvin torcolvin commented Oct 8, 2024

Created new tests to cover import cases https://docs.google.com/document/d/19mC9-TptN7ynuF5XR9TQ_MckHWsSrjUfI2p71G_DD1U/edit#heading=h.xzdilbaiwy25

The specific bugs initially noticed were around _vv.ver getting updated to newVVEntry.Value = expandMacroCASValueUint64 which was never correct on import. Additional logic was expanded to cover all tests.

For ease of reading this code, the existing tests were ported to a parameterized test first with the buggy logic in place, you can read the commit separately.

Some tests are disabled are set to fix at a future time https://jira.issues.couchbase.com/browse/CBG-4292. Before working on that ticket, https://jira.issues.couchbase.com/browse/CBG-4291 should be fixed on main and rebased onto anemone.

Code is also tested with live XDCR.

Also removes ImportCAS as part of CBG-4285.

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

- remove _sync.ImportCAS (CBG-4285)
- don't update HLV if mutation comes from XDCR
@torcolvin torcolvin assigned adamcfraser and torcolvin and unassigned adamcfraser Oct 8, 2024
@torcolvin torcolvin marked this pull request as draft October 8, 2024 16:28
@torcolvin torcolvin marked this pull request as ready for review October 8, 2024 19:15
@torcolvin torcolvin assigned adamcfraser and unassigned torcolvin Oct 8, 2024
Copy link
Collaborator

@adamcfraser adamcfraser left a comment

Choose a reason for hiding this comment

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

Functionally looks ok, some suggestions for clarity on the comments/conditions.

// Set ImportCAS to the previous document CAS, but don't otherwise modify HLV
d.HLV.ImportCAS = d.Cas
} else {
// Update HLV if:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the description as you've got it muddies the rationale behind the decision making here (simply that we want to update the HLV only if the current document cas isn't already accounted for in the HLV + mou).

Suggested change
// Update HLV if:
// Do not update HLV if the current document version (cas) is already included in the existing HLV,
// as either:
// 1. _vv.cvCAS == document.cas (current mutation is already present as cv), or
// 2. _mou.cas == document.cas (current mutation is already present as cv, and was imported on a different cluster)

// - document was written by another cluster, itself having an undergone an import
// - _vv.cvCAS == document cas and _mou.cas != document cas

if !hasHLV || (d.HLV.CurrentVersionCAS != d.Cas && (previousMou == nil || base.HexCasToUint64(previousMou.CAS) != d.Cas)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this can be rewritten to match the comment suggestion I think that would be more readable. Even something like this maybe:

cvCASMatch := hasHLV && d.HLV.CurrentVersionCAS == d.Cas
mouMatch := previousMou != nil && base.HexCasToUint64(previousMou.CAS) == d.Cas)
if !cvCASMatch && !mouMatch {

@adamcfraser adamcfraser assigned torcolvin and unassigned adamcfraser Oct 9, 2024
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