From 78f8360ca649d3885d13b248064e8bed7cfe9882 Mon Sep 17 00:00:00 2001 From: Abhinav Dangeti Date: Tue, 17 Dec 2024 10:18:32 -0700 Subject: [PATCH 1/3] MB-64604: [BP] Fix interpreting scorch config: "fieldTFRCacheThreshold" + Setting the default to 0 on account of the panics caught in the MB. + Firstly, refactor FieldTFRCacheThreshold to fieldTFRCacheThreshold for some naming consistency here. + This threshold can be used to toggle recycling of TermFieldReaders on/off. + Couchbase users will have the ability to provide this setting within a JSON payload, which when interpreted into a map[string]interface{} will need to be interpreted as a float64. + Should library users set it as an int within the index config - we'll honor that setting as well. --- index/scorch/snapshot_index.go | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/index/scorch/snapshot_index.go b/index/scorch/snapshot_index.go index fd5413236..05f60a734 100644 --- a/index/scorch/snapshot_index.go +++ b/index/scorch/snapshot_index.go @@ -51,15 +51,6 @@ type asynchSegmentResult struct { var reflectStaticSizeIndexSnapshot int -// DefaultFieldTFRCacheThreshold limits the number of TermFieldReaders(TFR) for -// a field in an index snapshot. Without this limit, when recycling TFRs, it is -// possible that a very large number of TFRs may be added to the recycle -// cache, which could eventually lead to significant memory consumption. -// This threshold can be overwritten by users at the library level by changing the -// exported variable, or at the index level by setting the FieldTFRCacheThreshold -// in the kvConfig. -var DefaultFieldTFRCacheThreshold uint64 = 10 - func init() { var is interface{} = IndexSnapshot{} reflectStaticSizeIndexSnapshot = int(reflect.TypeOf(is).Size()) @@ -567,10 +558,27 @@ func (i *IndexSnapshot) allocTermFieldReaderDicts(field string) (tfr *IndexSnaps } } -func (i *IndexSnapshot) getFieldTFRCacheThreshold() uint64 { +// DefaultFieldTFRCacheThreshold limits the number of TermFieldReaders(TFR) for +// a field in an index snapshot. Without this limit, when recycling TFRs, it is +// possible that a very large number of TFRs may be added to the recycle +// cache, which could eventually lead to significant memory consumption. +// This threshold can be overwritten by users at the library level by changing the +// exported variable, or at the index level by setting the "fieldTFRCacheThreshold" +// in the kvConfig. +var DefaultFieldTFRCacheThreshold int = 0 // disabled because it causes MB-64604 + +func (i *IndexSnapshot) getFieldTFRCacheThreshold() int { if i.parent.config != nil { - if _, ok := i.parent.config["FieldTFRCacheThreshold"]; ok { - return i.parent.config["FieldTFRCacheThreshold"].(uint64) + if _, exists := i.parent.config["fieldTFRCacheThreshold"]; exists { + val := i.parent.config["fieldTFRCacheThreshold"] + if x, ok := val.(float64); ok { + // JSON unmarshal-ed into a map[string]interface{} will default + // to float64 for numbers, so we need to check for float64 first. + return int(x) + } else if x, ok := val.(int); ok { + // If library users provided an int in the config, we'll honor it. + return x + } } } return DefaultFieldTFRCacheThreshold @@ -597,7 +605,7 @@ func (i *IndexSnapshot) recycleTermFieldReader(tfr *IndexSnapshotTermFieldReader if i.fieldTFRs == nil { i.fieldTFRs = map[string][]*IndexSnapshotTermFieldReader{} } - if uint64(len(i.fieldTFRs[tfr.field])) < i.getFieldTFRCacheThreshold() { + if len(i.fieldTFRs[tfr.field]) < i.getFieldTFRCacheThreshold() { i.fieldTFRs[tfr.field] = append(i.fieldTFRs[tfr.field], tfr) } i.m2.Unlock() From 4da0d05bd5f86ff0d7ca2afda7c4527c92baf185 Mon Sep 17 00:00:00 2001 From: Abhinav Dangeti Date: Tue, 17 Dec 2024 10:35:31 -0700 Subject: [PATCH 2/3] Update workflows --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index b8a70ef5c..e0db47ee1 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -8,7 +8,7 @@ jobs: test: strategy: matrix: - go-version: [1.16.x, 1.17.x, 1.18.x] + go-version: [1.20.x, 1.21.x, 1.22.x] platform: [ubuntu-latest, macos-latest, windows-latest] runs-on: ${{ matrix.platform }} steps: From 608d25590110deffc4cee1dbeeb531a3942d09dc Mon Sep 17 00:00:00 2001 From: Abhinav Dangeti Date: Tue, 17 Dec 2024 10:52:13 -0700 Subject: [PATCH 3/3] Fix unnecessary additional lookup --- index/scorch/snapshot_index.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/index/scorch/snapshot_index.go b/index/scorch/snapshot_index.go index 05f60a734..88093ce49 100644 --- a/index/scorch/snapshot_index.go +++ b/index/scorch/snapshot_index.go @@ -569,8 +569,7 @@ var DefaultFieldTFRCacheThreshold int = 0 // disabled because it causes MB-64604 func (i *IndexSnapshot) getFieldTFRCacheThreshold() int { if i.parent.config != nil { - if _, exists := i.parent.config["fieldTFRCacheThreshold"]; exists { - val := i.parent.config["fieldTFRCacheThreshold"] + if val, exists := i.parent.config["fieldTFRCacheThreshold"]; exists { if x, ok := val.(float64); ok { // JSON unmarshal-ed into a map[string]interface{} will default // to float64 for numbers, so we need to check for float64 first.