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.

Sign up for GitHub

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

Jump to bottom

CBG-4134: link rev cache memory limit config option to rev cache #7084

Merged
merged 5 commits into from
Sep 6, 2024
Merged

CBG-4134: link rev cache memory limit config option to rev cache #7084

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
33fbcdd
CBG-4134: link rev cache memory limit config option to rev cache
gregns1 Aug 21, 2024
98151c2
failing tests
gregns1 Aug 21, 2024
773cd4d
address commnets
gregns1 Sep 4, 2024
69b3190
fix yaml lint
gregns1 Sep 4, 2024
1fd6027
fix failing test + mistake in docs
gregns1 Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/database_config/ee-cache-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"bucket": "default",
"cache": {
"rev_cache": {
"size": 5000,
"max_item_count": 5000,
"shard_count": 8
},
"channel_cache": {
Expand Down
2 changes: 1 addition & 1 deletion examples/legacy_config/ee-cache-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
"enable_star_channel":true
},
"rev_cache":{
"size":5000,
"max_item_count":5000,
"shard_count":8
}
}
Expand Down
2 changes: 1 addition & 1 deletion rest/access_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ func TestUserHasDocAccessDocNotFound(t *testing.T) {
QueryPaginationLimit: base.IntPtr(2),
CacheConfig: &CacheConfig{
RevCacheConfig: &RevCacheConfig{
Size: base.Uint32Ptr(0),
MaxItemCount: base.Uint32Ptr(0),
},
ChannelCacheConfig: &ChannelCacheConfig{
MaxNumber: base.IntPtr(0),
Expand Down
4 changes: 2 additions & 2 deletions rest/adminapitest/admin_api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2547,7 +2547,7 @@ func TestHandleDBConfig(t *testing.T) {
dbConfig := rt.NewDbConfig()
dbConfig.CacheConfig = &rest.CacheConfig{
RevCacheConfig: &rest.RevCacheConfig{
Size: base.Uint32Ptr(1337), ShardCount: base.Uint16Ptr(7),
MaxItemCount: base.Uint32Ptr(1337), ShardCount: base.Uint16Ptr(7),
},
}

Expand All @@ -2567,7 +2567,7 @@ func TestHandleDBConfig(t *testing.T) {

gotRevcache, ok := gotcache["rev_cache"].(map[string]interface{})
require.True(t, ok)
gotRevcacheSize, ok := gotRevcache["size"].(json.Number)
gotRevcacheSize, ok := gotRevcache["max_item_count"].(json.Number)
require.True(t, ok)
gotRevcacheSizeInt, err := gotRevcacheSize.Int64()
require.NoError(t, err)
Expand Down
6 changes: 3 additions & 3 deletions rest/bootstrap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func TestBootstrapRESTAPISetup(t *testing.T) {

// upsert 1 config field
resp = BootstrapAdminRequest(t, sc, http.MethodPost, "/db1/_config",
`{"cache": {"rev_cache":{"size":1234}}}`,
`{"cache": {"rev_cache":{"max_item_count":1234}}}`,
)
resp.RequireStatus(http.StatusCreated)

Expand All @@ -69,7 +69,7 @@ func TestBootstrapRESTAPISetup(t *testing.T) {
assert.Empty(t, dbConfigResp.Username)
assert.Empty(t, dbConfigResp.Password)
require.Nil(t, dbConfigResp.Sync)
require.Equal(t, uint32(1234), *dbConfigResp.CacheConfig.RevCacheConfig.Size)
require.Equal(t, uint32(1234), *dbConfigResp.CacheConfig.RevCacheConfig.MaxItemCount)

// Sanity check to use the database
resp = BootstrapAdminRequest(t, sc, http.MethodPut, "/db1/doc1", `{"foo":"bar"}`)
Expand Down Expand Up @@ -103,7 +103,7 @@ func TestBootstrapRESTAPISetup(t *testing.T) {
assert.Empty(t, dbConfigResp.Username)
assert.Empty(t, dbConfigResp.Password)
require.Nil(t, dbConfigResp.Sync)
require.Equal(t, uint32(1234), *dbConfigResp.CacheConfig.RevCacheConfig.Size)
require.Equal(t, uint32(1234), *dbConfigResp.CacheConfig.RevCacheConfig.MaxItemCount)

// Ensure it's _actually_ the same bucket
resp = BootstrapAdminRequest(t, sc, http.MethodGet, "/db1/doc1", ``)
Expand Down
20 changes: 13 additions & 7 deletions rest/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,8 +246,9 @@ type DeprecatedCacheConfig struct {
}

type RevCacheConfig struct {
gregns1 marked this conversation as resolved.
Show resolved Hide resolved
Size *uint32 `json:"size,omitempty"` // Maximum number of revisions to store in the revision cache
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't remove config options like this, it would be a breaking change for customers with existing configuration.

If we want to keep the size JSON field name, and rename the internal struct field to MaxItemCount to better represent the behaviour in the codebase, we can do that. The alternatve would be adding a new property, deprecating Size but still allowing it as a fallback.

Both of these options come with downsides.

ShardCount *uint16 `json:"shard_count,omitempty"` // Number of shards the rev cache should be split into
MaxItemCount *uint32 `json:"max_item_count,omitempty"` // Maximum number of revisions to store in the revision cache
MaxMemoryCountMB *uint32 `json:"max_memory_count_mb"` // Maximum amount of memory the rev cache should consume in MB
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the behaviour when both of these are set? The lower of the two?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, both will work in tandem with one another when memory limit is set. I have added a comment to MaxMemoryCountMB to reflect this info, let me know if it needs work.

ShardCount *uint16 `json:"shard_count,omitempty"` // Number of shards the rev cache should be split into
}

type ChannelCacheConfig struct {
Expand Down Expand Up @@ -814,10 +815,15 @@ func (dbConfig *DbConfig) validateVersion(ctx context.Context, isEnterpriseEditi

if dbConfig.CacheConfig.RevCacheConfig != nil {
// EE: disable revcache
revCacheSize := dbConfig.CacheConfig.RevCacheConfig.Size
revCacheSize := dbConfig.CacheConfig.RevCacheConfig.MaxItemCount
if !isEnterpriseEdition && revCacheSize != nil && *revCacheSize == 0 {
base.WarnfCtx(ctx, eeOnlyWarningMsg, "cache.rev_cache.size", *revCacheSize, db.DefaultRevisionCacheSize)
dbConfig.CacheConfig.RevCacheConfig.Size = nil
base.WarnfCtx(ctx, eeOnlyWarningMsg, "cache.rev_cache.max_item_count", *revCacheSize, db.DefaultRevisionCacheSize)
dbConfig.CacheConfig.RevCacheConfig.MaxItemCount = nil
}
revCacheMemoryLimit := dbConfig.CacheConfig.RevCacheConfig.MaxMemoryCountMB
if !isEnterpriseEdition && revCacheMemoryLimit != nil && *revCacheMemoryLimit != 0 {
base.WarnfCtx(ctx, eeOnlyWarningMsg, "cache.rev_cache.max_memory_count_mb", *revCacheMemoryLimit, "no memory limit")
dbConfig.CacheConfig.RevCacheConfig.MaxMemoryCountMB = nil
}

if dbConfig.CacheConfig.RevCacheConfig.ShardCount != nil {
Expand Down Expand Up @@ -1116,8 +1122,8 @@ func (dbConfig *DbConfig) deprecatedConfigCacheFallback() (warnings []string) {
}

if dbConfig.DeprecatedRevCacheSize != nil {
if dbConfig.CacheConfig.RevCacheConfig.Size == nil {
dbConfig.CacheConfig.RevCacheConfig.Size = dbConfig.DeprecatedRevCacheSize
if dbConfig.CacheConfig.RevCacheConfig.MaxItemCount == nil {
dbConfig.CacheConfig.RevCacheConfig.MaxItemCount = dbConfig.DeprecatedRevCacheSize
}
warnings = append(warnings, fmt.Sprintf(warningMsgFmt, "rev_cache_size", "cache.rev_cache.size"))
}
Expand Down
4 changes: 2 additions & 2 deletions rest/config_database.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,8 @@ func DefaultDbConfig(sc *StartupConfig, useXattrs bool) *DbConfig {
AllowEmptyPassword: base.BoolPtr(false),
CacheConfig: &CacheConfig{
RevCacheConfig: &RevCacheConfig{
Size: base.Uint32Ptr(db.DefaultRevisionCacheSize),
ShardCount: base.Uint16Ptr(db.DefaultRevisionCacheShardCount),
MaxItemCount: base.Uint32Ptr(db.DefaultRevisionCacheSize),
ShardCount: base.Uint16Ptr(db.DefaultRevisionCacheShardCount),
},
ChannelCacheConfig: &ChannelCacheConfig{
MaxNumber: base.IntPtr(db.DefaultChannelCacheMaxNumber),
Expand Down
52 changes: 45 additions & 7 deletions rest/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"crypto/rsa"
"crypto/tls"
"crypto/x509"
"encoding/json"
"errors"
"flag"
"fmt"
Expand Down Expand Up @@ -176,7 +177,7 @@ func TestConfigValidationDeltaSync(t *testing.T) {
}

func TestConfigValidationCache(t *testing.T) {
jsonConfig := `{"databases": {"db": {"cache": {"rev_cache": {"size": 0}, "channel_cache": {"max_number": 100, "compact_high_watermark_pct": 95, "compact_low_watermark_pct": 25}}}}}`
jsonConfig := `{"databases": {"db": {"cache": {"rev_cache": {"max_item_count": 0}, "channel_cache": {"max_number": 100, "compact_high_watermark_pct": 95, "compact_low_watermark_pct": 25}}}}}`
ctx := base.TestCtx(t)
buf := bytes.NewBufferString(jsonConfig)
config, err := readLegacyServerConfig(ctx, buf)
Expand All @@ -190,11 +191,11 @@ func TestConfigValidationCache(t *testing.T) {

require.NotNil(t, config.Databases["db"].CacheConfig.RevCacheConfig)
if base.IsEnterpriseEdition() {
require.NotNil(t, config.Databases["db"].CacheConfig.RevCacheConfig.Size)
assert.Equal(t, 0, int(*config.Databases["db"].CacheConfig.RevCacheConfig.Size))
require.NotNil(t, config.Databases["db"].CacheConfig.RevCacheConfig.MaxItemCount)
assert.Equal(t, 0, int(*config.Databases["db"].CacheConfig.RevCacheConfig.MaxItemCount))
} else {
// CE disallowed - should be nil
assert.Nil(t, config.Databases["db"].CacheConfig.RevCacheConfig.Size)
assert.Nil(t, config.Databases["db"].CacheConfig.RevCacheConfig.MaxItemCount)
}

require.NotNil(t, config.Databases["db"].CacheConfig.ChannelCacheConfig)
Expand Down Expand Up @@ -488,7 +489,7 @@ func TestDeprecatedCacheConfig(t *testing.T) {
require.Len(t, warnings, 8)

// Check that the deprecated values have correctly been propagated upto the new config values
assert.Equal(t, *dbConfig.CacheConfig.RevCacheConfig.Size, uint32(10))
assert.Equal(t, *dbConfig.CacheConfig.RevCacheConfig.MaxItemCount, uint32(10))
assert.Equal(t, *dbConfig.CacheConfig.ChannelCacheConfig.ExpirySeconds, 10)
assert.Equal(t, *dbConfig.CacheConfig.ChannelCacheConfig.MinLength, 10)
assert.Equal(t, *dbConfig.CacheConfig.ChannelCacheConfig.MaxLength, 10)
Expand All @@ -507,7 +508,7 @@ func TestDeprecatedCacheConfig(t *testing.T) {

// Set A Couple Deprecated Values AND Their New Counterparts
dbConfig.DeprecatedRevCacheSize = base.Uint32Ptr(10)
dbConfig.CacheConfig.RevCacheConfig.Size = base.Uint32Ptr(20)
dbConfig.CacheConfig.RevCacheConfig.MaxItemCount = base.Uint32Ptr(20)
dbConfig.CacheConfig.DeprecatedEnableStarChannel = base.BoolPtr(false)
dbConfig.CacheConfig.ChannelCacheConfig.EnableStarChannel = base.BoolPtr(true)

Expand All @@ -518,7 +519,7 @@ func TestDeprecatedCacheConfig(t *testing.T) {
require.Len(t, warnings, 2)

// Check that the deprecated value has been ignored as the new value is the priority
assert.Equal(t, *dbConfig.CacheConfig.RevCacheConfig.Size, uint32(20))
assert.Equal(t, *dbConfig.CacheConfig.RevCacheConfig.MaxItemCount, uint32(20))
assert.Equal(t, *dbConfig.CacheConfig.ChannelCacheConfig.EnableStarChannel, true)
}

Expand Down Expand Up @@ -3067,3 +3068,40 @@ func TestNotFoundOnInvalidDatabase(t *testing.T) {
assert.Equal(c, 1, len(rt.ServerContext().dbConfigs))
}, time.Second*10, time.Millisecond*100)
}

func TestRevCacheMemoryLimitConfig(t *testing.T) {
rt := NewRestTester(t, &RestTesterConfig{
CustomTestBucket: base.GetTestBucket(t),
PersistentConfig: true,
})
defer rt.Close()

dbConfig := rt.NewDbConfig()
RequireStatus(t, rt.CreateDatabase("db1", dbConfig), http.StatusCreated)

resp := rt.SendAdminRequest(http.MethodGet, "/db1/_config", "")
RequireStatus(t, resp, http.StatusOK)

require.NoError(t, json.Unmarshal(resp.BodyBytes(), &dbConfig))
assert.Nil(t, dbConfig.CacheConfig)

dbConfig.CacheConfig = &CacheConfig{}
dbConfig.CacheConfig.RevCacheConfig = &RevCacheConfig{
MaxItemCount: base.Uint32Ptr(100),
MaxMemoryCountMB: base.Uint32Ptr(4),
}
RequireStatus(t, rt.UpsertDbConfig("db1", dbConfig), http.StatusCreated)

resp = rt.SendAdminRequest(http.MethodGet, "/db1/_config", "")
RequireStatus(t, resp, http.StatusOK)

require.NoError(t, json.Unmarshal(resp.BodyBytes(), &dbConfig))
assert.NotNil(t, dbConfig.CacheConfig)

assert.Equal(t, uint32(100), *dbConfig.CacheConfig.RevCacheConfig.MaxItemCount)
if base.IsEnterpriseEdition() {
assert.Equal(t, uint32(4), *dbConfig.CacheConfig.RevCacheConfig.MaxMemoryCountMB)
} else {
assert.Nil(t, dbConfig.CacheConfig.RevCacheConfig.MaxMemoryCountMB)
}
}
6 changes: 3 additions & 3 deletions rest/revocation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,7 +1087,7 @@ func TestRevocationsWithQueryLimit(t *testing.T) {
QueryPaginationLimit: base.IntPtr(2),
CacheConfig: &CacheConfig{
RevCacheConfig: &RevCacheConfig{
Size: base.Uint32Ptr(0),
MaxItemCount: base.Uint32Ptr(0),
},
ChannelCacheConfig: &ChannelCacheConfig{
MaxNumber: base.IntPtr(0),
Expand Down Expand Up @@ -1176,7 +1176,7 @@ func TestRevocationsWithQueryLimitChangesLimit(t *testing.T) {
QueryPaginationLimit: base.IntPtr(2),
CacheConfig: &CacheConfig{
RevCacheConfig: &RevCacheConfig{
Size: base.Uint32Ptr(0),
MaxItemCount: base.Uint32Ptr(0),
},
ChannelCacheConfig: &ChannelCacheConfig{
MaxNumber: base.IntPtr(0),
Expand Down Expand Up @@ -1227,7 +1227,7 @@ func TestRevocationUserHasDocAccessDocNotFound(t *testing.T) {
QueryPaginationLimit: base.IntPtr(2),
CacheConfig: &CacheConfig{
RevCacheConfig: &RevCacheConfig{
Size: base.Uint32Ptr(0),
MaxItemCount: base.Uint32Ptr(0),
},
ChannelCacheConfig: &ChannelCacheConfig{
MaxNumber: base.IntPtr(0),
Expand Down
7 changes: 5 additions & 2 deletions rest/server_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1164,8 +1164,11 @@ func dbcOptionsFromConfig(ctx context.Context, sc *ServerContext, config *DbConf
}

if config.CacheConfig.RevCacheConfig != nil {
if config.CacheConfig.RevCacheConfig.Size != nil {
revCacheOptions.MaxItemCount = *config.CacheConfig.RevCacheConfig.Size
if config.CacheConfig.RevCacheConfig.MaxItemCount != nil {
revCacheOptions.MaxItemCount = *config.CacheConfig.RevCacheConfig.MaxItemCount
}
if config.CacheConfig.RevCacheConfig.MaxMemoryCountMB != nil {
revCacheOptions.MaxBytes = int64(*config.CacheConfig.RevCacheConfig.MaxMemoryCountMB * 1024 * 1024) // Convert MB input to bytes
}
if config.CacheConfig.RevCacheConfig.ShardCount != nil {
revCacheOptions.ShardCount = *config.CacheConfig.RevCacheConfig.ShardCount
Expand Down
Loading