-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[Couchbase] Upgrade Server and Sync Gateway versions #21284
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
base: master
Are you sure you want to change the base?
Conversation
Remove Couchbase version 6.6, which was EOL in Oct 2023. Add version 7.2 and 7.6, EOL in Jul 2026 and Mar 2027, respectively. Keep version 7.0 and 7.1, even though they were EOL in July 2025. This keeps some backwards compatibility, and also reflects Couchbase's own documentation pages, where 7.0 and 7.1 are still available to browse. Discontinue the cbdocloader, which is not available in version 7.6. Instead, use the REST endpoint /sampleBuckets/install. Remove the expected index stat tag "bucket:cb_bucket", as it is not present in the index stats for either version. It's not clear how that ever worked. Instead, rely on the "bucket:gamesim-sample" tag. Add two index metrics: ``` couchbase.index.avg_array_length couchbase.index.docid_count ``` These have been around for years since upstream commit 055dff4 [1], and seem to only conditionally show up if array indexes are used, so they probably went unnoticed. [1]: MB-32633: Add new index stats for array index couchbase/indexing@055dff4
Version 2.8 was EOL in Aug 2023; update to 3.3 with EOL Jul 2028. A database now has to be created explicitly, so do that in the tests. Modify the bootstrap parameters to be explicit about disabling TLS in tests. Use the recommended metrics port, 4986. Note the removal of the following metrics, which don't seem to be present in Sync Gateway 3.3.0, even though the documentation mentions them: ``` couchbase.sync_gateway.cbl_replication_push.sync_function_count couchbase.sync_gateway.cbl_replication_push.sync_function_time couchbase.sync_gateway.database.abandoned_seqs couchbase.sync_gateway.database.cache_feed.dcp_backfill_completed couchbase.sync_gateway.database.cache_feed.dcp_backfill_expected couchbase.sync_gateway.database.cache_feed.dcp_rollback_count couchbase.sync_gateway.database.import_feed.dcp_backfill_completed couchbase.sync_gateway.database.import_feed.dcp_backfill_expected couchbase.sync_gateway.database.import_feed.dcp_rollback_count ``` These were cross-referenced and confirmed to be missing in the actual code [1], meaning the documentation is quite certainly wrong. [1]: https://github.com/couchbase/sync_gateway/blob/release/3.3.0/base/stats.go
Codecov Report❌ Patch coverage is Additional details and impacted files🚀 New features to boost your workflow:
|
Address PR check issues: ``` $ ddev validate metadata couchbase --sync couchbase: metadata.csv is not sorted by metric name. Sorting couchbase/metadata.csv by metric names ``` Truncate a description: ``` couchbase:360 `couchbase.sync_gateway.database.sync_function_time` description exceeds the max length: 400 for descriptions. ``` The sync also updates the CSV schema.
👋 thanks for the PR. I asked the team owning this part of the code to have a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR 🥳 !
Just have a small comment about the changelog and a non-blocking question. We can get it out as soon as they are addressed.
@@ -7,10 +7,21 @@ services: | |||
container_name: ${CB_CONTAINER_NAME} | |||
couchbase-sync-gateway: | |||
container_name: couchbase-sync-gateway | |||
image: couchbase/sync-gateway:2.8.3-enterprise | |||
image: couchbase/sync-gateway:3.3.0-enterprise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking: Is this image valid for all cauchbase versions? Should we have this variable setup in hatch.toml as well? If yes, we might want to have custom commands per gateway version and load them as volumes. I am not sure about this since I am lacking context on Couchbase at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spotted! I had missed the compatibility matrix. A fix has been pushed that uses Sync Gateway 3.1.0 for Couchbase 7.0, plus conditional handling of the different metrics it exposes.
couchbase/changelog.d/21284.added
Outdated
@@ -0,0 +1 @@ | |||
[Couchbase] Upgrade Server and Sync Gateway versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocking: can we remove the [Couchbase]
prefix? This is the changelog for Couchbase which seems to provide all the context we need.
Can we mention in the changelog that we are adding new metrics with som context about what these metrics are?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix pushed. Prefix removed, and additional metrics explained.
As raised in the PR, the version compatibility [1] between Sync Gateway and Server should be taken into account. Couchbase Server 7.0 needs Sync Gateway 3.1.0. There's around 35 more metrics in 3.3.0 vs 3.1.0, so make the testing for those dependent on version. Also deal with a version difference for database creation, where 3.1.0's payload should use num_index_replicas, while 3.3.0 requires index.num_replicas. [1]: Couchbase Sync Gateway / Product Notes / Compatibility Matrix: https://docs.couchbase.com/sync-gateway/current/product-notes/compatibility.html
What does this PR do?
Upgrade Couchbase Server and Sync Gateway to current versions; add a few more metrics.
Motivation
This upgrade paves the way for transitioning to Prometheus metrics, which are far more comprehensive than the legacy metrics. I intend to file another PR for that once this one has been accepted, because it's easier to work on that transition without having to deal with the ancient Server version 6.6.
Question: Do I modify the integration's version myself?
Additional implementation details in the commit messages.
CC @tbavelier @FlorentClarret
Review checklist (to be filled by reviewers)
qa/skip-qa
label if the PR doesn't need to be tested during QA.backport/<branch-name>
label to the PR and it will automatically open a backport PR once this one is merged