CBG-4895: Allow tuning of dcp_read_buffer and kv_buffer server connstr parameters #7829

RIT3shSapata · 2025-10-17T09:53:26Z

CBG-4895

Describe your PR here...

Currently dcp_read_buffer and kv_buffer are only supported if CBS is serverless, need to enable them for regular CBS as well for perf tests in capella
remove serverless check for unsupported config options such as dcp_read_buffer and kv_buffer

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

GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGatewayIntegration/142/
GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGatewayIntegration/146/
GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGatewayIntegration/147/

Copilot

Pull Request Overview

This PR removes the serverless-only gating of dcp_read_buffer and kv_buffer connection string parameters so they can be tuned for regular Couchbase Server deployments, and adds a test to validate expected connection string formation.

Removes the IsServerless() conditional, applying DefaultServerlessGoCBConnStringParams unconditionally.
Adds a new test (TestUnsupportedOptions) covering explicit and default parameter cases in a serverless test configuration.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
rest/server_context.go	Removes serverless check so serverless-specific default connection params are always applied; enables unsupported tuning options for all deployments.
rest/api_test.go	Adds TestUnsupportedOptions to assert connection string parameters with and without explicitly provided unsupported options.

rest/server_context.go

Copilot · 2025-10-17T09:54:30Z

rest/api_test.go

 	base.DebugfCtx(t.Context(), base.KeySGTest, "additional logs")
 }
+
+func TestUnsupportedOptions( t *testing.T){


Extraneous space after '(' in the test function signature deviates from standard Go formatting (gofmt would produce func TestUnsupportedOptions(t *testing.T)). Remove the space to keep consistency.

Suggested change

func TestUnsupportedOptions( t *testing.T){

func TestUnsupportedOptions(t *testing.T){

Copilot · 2025-10-17T09:54:31Z

rest/api_test.go

+					tb.GetName(), base.TestsDisableGSI()))
+				RequireStatus(t, resp, http.StatusCreated)
+			}
+			assert.Equal(t, test.expectedConnStr, sc.getBucketSpec("db").Server)


[nitpick] The test asserts exact string equality of the full connection string, making it brittle to harmless reordering of query parameters. Consider parsing the query portion and asserting key-value presence (e.g. using url.Parse + comparison of a map) rather than relying on order-specific full-string matching.

rest/server_context.go

rest/api_test.go

torcolvin · 2025-10-17T13:25:42Z

rest/api_test.go

 }
+
+func TestUnsupportedOptions(t *testing.T) {
+	RequireBucketSpecificCredentials(t)


RequireBucketSpecificCredentials should be changed to specify that it needs CBS RBAC credentials.

This test probably needs SG_TEST_BACKING_STORE=couchbase to fall into the gocb sections, but it would actually be served by a function that said:

if base.UnitTestUrlIsWalrus() { t.Skip("This test only works against Couchbase Server since it requires a gocb bucket") }

I don't think this is applicable, given, the unit test has been refactored to directly generate the bucketspec

torcolvin · 2025-10-24T12:54:39Z

rest/server_context.go

-	if serverConfig.IsServerless() {
-		params := base.DefaultServerlessGoCBConnStringParams()
+
+	if strings.HasPrefix(server, "couchbase") {


Suggested change

if strings.HasPrefix(server, "couchbase") {

if !base.ServerIsWalrus(server) {

torcolvin · 2025-10-27T14:51:02Z

rest/server_context.go

 	}

-	if !base.ServerIsWalrus(server) {
+	if !base.ServerIsWalrus(server) && server != "" {


This is production code so I'm cautious about modifying it, what's an example test that fails? It feels like we should define a Server in all cases.

TestDeleteDatabasePointingAtSameBucket kept failing if I did not add server != "". When I dug into it, the test tries to create a db on the same bucket.
When the createDB handler handles this request and then tries to setup the config passed here, it updates the config string to "" from nil, while inheriting the connection string from bootstrapConfig, here. Since this is a unit test, so we do not set the bootstrap config, therefore, I think it is expected for the Config.Server to be set to "".
This is why I added this additional check, I know this condition looks very suspicious, but I'm not not sure as to how else do I prevent the test from failing

A slight correction to my comment earlier,

while inheriting the connection string from bootstrapConfig, here. Since this is a unit test, so we do not set the bootstrap config, therefore, I think it is expected for the Config.Server to be set to "".

It does not inherit the existing Bootstrap config, instead it inherits an empty bootstrap config

RIT3shSapata · 2025-10-30T08:45:02Z

rest/server_context.go


 	var server string
-	if config.Server != nil {
+	if config.Server != nil && *config.Server != "" {


I've moved the check for empty string from _getOrAddDatabaseFromConfig, i'm doing the check inside the GetBucketSpec

remove serverless check for unsupported config

5288883

RIT3shSapata requested a review from torcolvin October 17, 2025 09:53

RIT3shSapata assigned torcolvin Oct 17, 2025

Copilot AI review requested due to automatic review settings October 17, 2025 09:53

Copilot AI reviewed Oct 17, 2025

View reviewed changes

lint fix

9a49b47

torcolvin reviewed Oct 17, 2025

View reviewed changes

torcolvin assigned RIT3shSapata and unassigned torcolvin Oct 17, 2025

add more test cases and refactor unit test to run faster

ad83f57

RIT3shSapata requested a review from torcolvin October 23, 2025 16:43

RIT3shSapata assigned torcolvin and unassigned RIT3shSapata Oct 23, 2025

torcolvin assigned RIT3shSapata and unassigned torcolvin Oct 23, 2025

prevent adding connection params for rosmar and walrus

f4650d3

RIT3shSapata assigned torcolvin and unassigned RIT3shSapata Oct 24, 2025

remove check for http

296da24

torcolvin reviewed Oct 24, 2025

View reviewed changes

change server check condition

b501c94

RIT3shSapata requested a review from torcolvin October 24, 2025 13:24

torcolvin assigned RIT3shSapata and unassigned torcolvin Oct 24, 2025

fix for empty connection string check

dfee3e5

torcolvin reviewed Oct 27, 2025

View reviewed changes

refactor check for empty server value

e925166

RIT3shSapata commented Oct 30, 2025

View reviewed changes

RIT3shSapata requested a review from torcolvin October 30, 2025 08:46

RIT3shSapata assigned torcolvin Oct 30, 2025

RIT3shSapata removed their assignment Oct 30, 2025

	func TestUnsupportedOptions( t *testing.T){
	func TestUnsupportedOptions(t *testing.T){

	if strings.HasPrefix(server, "couchbase") {
	if !base.ServerIsWalrus(server) {

CBG-4895: Allow tuning of dcp_read_buffer and kv_buffer server connstr parameters #7829

Are you sure you want to change the base?

CBG-4895: Allow tuning of dcp_read_buffer and kv_buffer server connstr parameters #7829

Conversation

RIT3shSapata commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-review checklist

Dependencies (if applicable)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RIT3shSapata commented Oct 17, 2025 •

edited

Loading