CBG-4892: Add eccv to cluster_info endpoint #7846
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds Extended Cross-Cluster Versioning (ECCV) information to the _cluster_info endpoint. The endpoint now returns the ECCV setting for each bucket, which will be used by sgcollect_info to display bucket configurations.
Key changes:
- Added
EnableCrossClusterVersioningfield to theBucketInforesponse structure - Implemented
getBucketCCVSettings()method to retrieve ECCV settings for a given bucket - Added test coverage for the new ECCV functionality in
_cluster_infoendpoint
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rest/server_context.go | Implements helper method to retrieve ECCV settings for a bucket by iterating through databases |
| rest/admin_api.go | Updates BucketInfo struct and handleGetClusterInfo to include ECCV in the response |
| rest/api_test.go | Adds test to validate ECCV values are returned in _cluster_info endpoint response |
gregns1
left a comment
gregns1
left a comment
There was a problem hiding this comment.
Generally looks good just a few suggestions and the need for the eccv value to be presented in legacy config mode.
rest/api_test.go
Outdated
| } | ||
| var clusterInfo ClusterInfoResponse | ||
| err := base.JSONUnmarshal(resp.Body.Bytes(), &clusterInfo) | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
| assert.NoError(t, err) | |
| require.NoError(t, err) |
rest/admin_api.go
Outdated
| continue | ||
| } | ||
|
|
||
| eccv := h.server.getBucketCCVSettings(bucketName) |
There was a problem hiding this comment.
I would probably change this to avoid iterating multiple times through the map of database contexts. Maybe move this outside the bucket name iteration and have this function just iterate through database context map and return a map of bucket name to eccv value. Then you can use this map to lookup bucket name eccv value here.
This just avoids iterating through db context map for each bucket name here.
| var clusterInfo ClusterInfoResponse | ||
| err := base.JSONUnmarshal(resp.Body.Bytes(), &clusterInfo) | ||
| assert.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
I would also add assertion to this to assert that the eccv value is true (given we set it to true for our test suite)
gregns1
left a comment
gregns1
left a comment
There was a problem hiding this comment.
Generally looks good just couple of small tweaks and should be ready to go!
rest/api_test.go
Outdated
| err := base.JSONUnmarshal(resp.Body.Bytes(), &clusterInfo) | ||
| require.NoError(t, err) | ||
| assert.True(t, clusterInfo.Buckets[bucketName].EnableCrossClusterVersioning) | ||
| //RequireStatus(t, rt.SendAdminRequest(http.MethodDelete, fmt.Sprintf("/%s/", tests.dbName), ""), http.StatusOK) |
There was a problem hiding this comment.
Should this commented out line be removed?
rest/admin_api.go
Outdated
|
|
||
| type BucketInfo struct { | ||
| Registry GatewayRegistry `json:"registry"` | ||
| Registry GatewayRegistry `json:"registry,omitempty"` |
There was a problem hiding this comment.
For omit empty to work correctly here I think you'll have to make this GatewayRegistry struct a pointer.
Running in legacy config mode as it stands prints empty registry for the bucket:
CBG-4892
Describe your PR here...
_cluster_infoendpointsgcollect_infoto display this setting of a bucketPre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiDependencies (if applicable)
Integration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGatewayIntegration/150/