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

Sync optimizations for changes and proposeChanges #6190

Merged
merged 7 commits into from
May 5, 2023

Conversation

snej
Copy link
Contributor

@snej snej commented Apr 13, 2023

CBG-2851

Describe your PR here...

  • Skip bucket Get when a new doc is pushed via BLIP. When a client pushes a doc and proposeChanges sees that the docID doesn't exist in the bucket, the rev handler will write the doc to the bucket as new without trying to get an existing copy first. This will usually succeed, saving time. If it fails, it's handled like any other CAS mismatch, by retrying and getting the existing doc.
    • Added a field BlipSyncContext.pendingInsertions, a set of docIDs.
    • RevDiff returns a new status ProposedRev_OK_IsNew.
    • When the proposeChanges handler gets ProposedRev_OK_IsNew, it treats it
      like ProposedRev_OK and adds the docID to pendingInsertions.
    • The rev handler then checks if the docID is in pendingInsertions; if so,
      it passes an empty BucketDoc with cas=0 to PutExistingRev as the
      existingDoc.
    • WriteUpdateWithXattr now allows a previous doc with cas=0, interpreting it
      as the document not previously existing.
  • Make RevDiff & CheckProposedRev read and unmarshal less.
    • Added a new DocUnmarshalLevel constant, DocUnmarshalHistory, which unmarshals
      just the history, current rev and CAS.
    • Added GetDocSyncDataNoImport, which is like GetDocSyncData but does not
      check whether on-demand import is needed (this lets it avoid reading the doc
      body) and takes a DocumentUnmarshalLevel parameter.
    • RevDiff and CheckProposedRev call this instead. RevDiff uses the level
      DocUnmarshalHistory, and CheckProposedRev uses DocUnmarshalRev unless it
      might need the history.

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

Dependencies (if applicable)

  • Link upstream PRs
  • Update Go module dependencies when merged

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.

This is not a thorough review from me, just spotted something that is not multi-collection aware.

@snej snej force-pushed the fix/optimize-changes branch from 35e4cd5 to 302b6ae Compare April 18, 2023 22:42
@snej snej requested review from torcolvin, bbrks and adamcfraser April 18, 2023 23:00
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.

Overall the enhancements look great, some questions on the details.

@snej snej requested a review from adamcfraser April 20, 2023 00:56
@snej snej requested a review from torcolvin April 21, 2023 17:01
@snej
Copy link
Contributor Author

snej commented Apr 21, 2023

Note that I fixed the test failure by having RevsDiff treat ErrXattrNotFound the same as NotFound ... I'm not sure that's correct. Should the ErrXattrNotFound be handled differently?

@adamcfraser adamcfraser self-assigned this Apr 27, 2023
@adamcfraser
Copy link
Collaborator

adamcfraser commented Apr 27, 2023

Note that I fixed the test failure by having RevsDiff treat ErrXattrNotFound the same as NotFound ... I'm not sure that's correct. Should the ErrXattrNotFound be handled differently?

The approach you've got makes sense for this use case - GetXattr differentiates between ErrNotFound and ErrXattrNotFound for use cases that want to handle them differently.

@adamcfraser
Copy link
Collaborator

Changes look good, but we should run integration tests before merging to make sure everything is clean when running against Couchbase Server. There were some fixes on master specifically around integration tests. @snej can you rebase on master and then kick off integration tests?

snej added 7 commits April 28, 2023 09:47
When a client pushes a doc and `proposeChanges` sees that the docID doesn't
exist in the bucket, the `rev` handler will write the doc to the bucket as new
without trying to get an existing copy first.

This will usually succeed, saving time. If it fails, it's handled like any
other CAS mismatch, by retrying and getting the existing doc.

- Added a field BlipSyncContext.pendingInsertions, a set of docIDs.
- RevDiff returns a new status `ProposedRev_OK_IsNew`.
- When the `proposeChanges` handler gets ProposedRev_OK_IsNew, it treats it
  like ProposedRev_OK and adds the docID to pendingInsertions.
- The `rev` handler then checks if the docID is in pendingInsertions; if so,
  it passes an empty BucketDoc with cas=0 to PutExistingRev as the
  `existingDoc`.
- WriteUpdateWithXattr now allows a `previous` doc with cas=0, interpreting it
  as the document not previously existing.
- Added a new DocUnmarshalLevel constant, DocUnmarshalHistory, which unmarshals
  just the history, current rev and CAS.
- Added GetDocSyncDataNoImport, which is like GetDocSyncData but does not
  check whether on-demand import is needed (this lets it avoid reading the doc
  body) and takes a DocumentUnmarshalLevel parameter.
- RevDiff and CheckProposedRev call this instead. RevDiff uses the level
  DocUnmarshalHistory, and CheckProposedRev uses DocUnmarshalRev unless it
  might need the history.
Also added a blipSyncCollectionContext reference to blipHandler, since several
methods were looking it up.
- Limit size of pendingInsertions
- GetDocSyncDataNoImport now upgrades a doc to xattrs
Also cleaned up GetDocSyncDataNoImport a bit
@snej snej force-pushed the fix/optimize-changes branch from 6569eac to 2e313e9 Compare April 28, 2023 16:48
@snej
Copy link
Contributor Author

snej commented Apr 28, 2023

@adamcfraser Unfortunately Jenkins says "snej is missing the Job/Build permission"...

@torcolvin
Copy link
Collaborator

@adamcfraser Unfortunately Jenkins says "snej is missing the Job/Build permission"...

I've granted you permission on jenkins.

@snej
Copy link
Contributor Author

snej commented May 1, 2023

I just kicked off a build, but it looks like Adam already ran the integration tests Friday and they passed.

@adamcfraser adamcfraser merged commit 6dee30f into master May 5, 2023
@adamcfraser adamcfraser deleted the fix/optimize-changes branch May 5, 2023 19:50
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.

3 participants