CBG-4806: move conflict detection to common function #7683
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes conflict detection logic by moving it from the BLIP tester client to a shared IsInConflict function in the db package. This ensures consistent conflict detection behavior between the blip tester client and PutExistingCurrentVersion operations.
- Extracts conflict detection logic into a reusable
IsInConflictfunction - Updates both BLIP tester client and CRUD operations to use the centralized function
- Adds comprehensive test coverage for the new conflict detection function
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
rest/utilities_testing_blip_client.go |
Replaces inline conflict detection with call to centralized IsInConflict function |
db/hybrid_logical_vector.go |
Adds new IsInConflict function and ErrNoNewVersionsToAdd error |
db/hybrid_logical_vector_test.go |
Adds test cases for the new IsInConflict function |
db/crud.go |
Updates PutExistingCurrentVersion to use the centralized conflict detection |
| // would respond to a SG changes message that CBL does not need this revision | ||
| if localHLV.DominatesSource(*incomingCV) { | ||
| inConflict, err := db.IsInConflict(nil, &localHLV, incomingHLV) | ||
| if err != nil { | ||
| require.FailNow(t, fmt.Sprintf("incoming CV %#+v has lower version than the local revision %#+v - this should've been filtered via changes response before ending up as a rev. blip tester would reply that to Sync Gateway that it doesn't need this revision", incomingCV, localHLV)) |
There was a problem hiding this comment.
The error handling logic is incorrect. The FailNow call with the 'lower version' message is triggered when any error occurs from IsInConflict, but according to the new function, errors can occur for multiple reasons including equal CVs or newer local versions. The error message should be updated to reflect the actual error or the error should be properly differentiated.
| require.FailNow(t, fmt.Sprintf("incoming CV %#+v has lower version than the local revision %#+v - this should've been filtered via changes response before ending up as a rev. blip tester would reply that to Sync Gateway that it doesn't need this revision", incomingCV, localHLV)) | |
| require.FailNow(t, fmt.Sprintf("IsInConflict error: %v (incoming CV %#+v, local revision %#+v)", err, incomingCV, localHLV)) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Will leave this as is given the extra context in original message.
| // below check to check local revision is newer than incoming revision will pass given the check and will add the | ||
| // incoming versions even if CVs are the same | ||
| if localCV.Equal(*incomingCV) { | ||
| base.TracefCtx(ctx, base.KeyVV, "incoming CV %#+v is equal to local revision %#+v", incomingCV, localCV) |
There was a problem hiding this comment.
I generally like %#+v for logging, but I think %#v might be better for HLV and CV since it displays versions in decimal instead of a hexadecimal.
Can you check this? If this is true, can you file a ticket to create a ruleguard rule so that we can detect this for the future?
If not, disregard this comment.
There was a problem hiding this comment.
It only seems to wrap the formatted bit into quotes.
Internally we handle the HLV in uint64 type so logging here for example should be fine although %#v seems to format a uint64 into hex.
There was a problem hiding this comment.
I think it's worth filing a ticket to evaluate the places we log this and make sure we don't log in hex, it just makes everything harder to handle. But I can assert https://github.com/couchbaselabs/sg_dev_tools/pull/37 will handle this too.
There was a problem hiding this comment.
CBG-4806
PutExistingCurrentVersionwork in the same way.Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiIntegration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGateway-Integration/33/