From 274bae116f5ca7ba7818f6cb1c372c486c5512ff Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Tue, 19 Mar 2019 00:47:35 +0000 Subject: [PATCH 01/12] Use a single index Results when querying across blocks --- .../integration/write_tagged_quorum_test.go | 10 +- .../server/tchannelthrift/node/service.go | 112 ++++++----- .../tchannelthrift/node/service_test.go | 61 +++--- src/dbnode/storage/index.go | 65 +----- src/dbnode/storage/index/block.go | 39 +++- src/dbnode/storage/index/block_prop_test.go | 27 ++- src/dbnode/storage/index/block_test.go | 189 +++++++++--------- src/dbnode/storage/index/results.go | 88 +++++++- src/dbnode/storage/index/results_test.go | 171 +++++++++------- src/dbnode/storage/index/types.go | 34 +++- .../storage/index_query_concurrent_test.go | 39 ++-- src/dbnode/storage/index_queue_test.go | 12 +- 12 files changed, 501 insertions(+), 346 deletions(-) diff --git a/src/dbnode/integration/write_tagged_quorum_test.go b/src/dbnode/integration/write_tagged_quorum_test.go index ca83a9e660..62d4476116 100644 --- a/src/dbnode/integration/write_tagged_quorum_test.go +++ b/src/dbnode/integration/write_tagged_quorum_test.go @@ -300,9 +300,13 @@ func nodeHasTaggedWrite(t *testing.T, s *testSetup) bool { require.NoError(t, err) results := res.Results require.Equal(t, testNamespaces[0].String(), results.Namespace().String()) - tags, ok := results.Map().Get(ident.StringID("quorumTest")) - idxFound := ok && ident.NewTagIterMatcher(ident.MustNewTagStringsIterator( - "foo", "bar", "boo", "baz")).Matches(ident.NewTagsIterator(tags)) + + var idxFound bool + results.WithMap(func(rMap *index.ResultsMap) { + tags, ok := rMap.Get(ident.StringID("quorumTest")) + idxFound = ok && ident.NewTagIterMatcher(ident.MustNewTagStringsIterator( + "foo", "bar", "boo", "baz")).Matches(ident.NewTagsIterator(tags)) + }) if !idxFound { return false diff --git a/src/dbnode/network/server/tchannelthrift/node/service.go b/src/dbnode/network/server/tchannelthrift/node/service.go index d303b77c70..c88fef5557 100644 --- a/src/dbnode/network/server/tchannelthrift/node/service.go +++ b/src/dbnode/network/server/tchannelthrift/node/service.go @@ -289,35 +289,45 @@ func (s *service) Query(tctx thrift.Context, req *rpc.QueryRequest) (*rpc.QueryR } result := &rpc.QueryResult_{ - Results: make([]*rpc.QueryResultElement, 0, queryResult.Results.Map().Len()), + Results: make([]*rpc.QueryResultElement, 0, queryResult.Results.Size()), Exhaustive: queryResult.Exhaustive, } + fetchData := true if req.NoData != nil && *req.NoData { fetchData = false } - for _, entry := range queryResult.Results.Map().Iter() { - elem := &rpc.QueryResultElement{ - ID: entry.Key().String(), - Tags: make([]*rpc.Tag, 0, len(entry.Value().Values())), - } - result.Results = append(result.Results, elem) - for _, tag := range entry.Value().Values() { - elem.Tags = append(elem.Tags, &rpc.Tag{ - Name: tag.Name.String(), - Value: tag.Value.String(), - }) - } - if !fetchData { - continue - } - tsID := entry.Key() - datapoints, err := s.readDatapoints(ctx, nsID, tsID, start, end, - req.ResultTimeType) - if err != nil { - return nil, convert.ToRPCError(err) + + var readErr error + queryResult.Results.WithMap(func(rMap *index.ResultsMap) { + for _, entry := range rMap.Iter() { + elem := &rpc.QueryResultElement{ + ID: entry.Key().String(), + Tags: make([]*rpc.Tag, 0, len(entry.Value().Values())), + } + result.Results = append(result.Results, elem) + for _, tag := range entry.Value().Values() { + elem.Tags = append(elem.Tags, &rpc.Tag{ + Name: tag.Name.String(), + Value: tag.Value.String(), + }) + } + if !fetchData { + continue + } + tsID := entry.Key() + datapoints, err := s.readDatapoints(ctx, nsID, tsID, start, end, + req.ResultTimeType) + if err != nil { + // Break for error + readErr = err + return + } + elem.Datapoints = datapoints } - elem.Datapoints = datapoints + }) + if readErr != nil { + return nil, convert.ToRPCError(readErr) } return result, nil @@ -422,33 +432,41 @@ func (s *service) FetchTagged(tctx thrift.Context, req *rpc.FetchTaggedRequest) results := queryResult.Results nsID := results.Namespace() tagsIter := ident.NewTagsIterator(ident.Tags{}) - for _, entry := range results.Map().Iter() { - tsID := entry.Key() - tags := entry.Value() - enc := s.pools.tagEncoder.Get() - ctx.RegisterFinalizer(enc) - tagsIter.Reset(tags) - encodedTags, err := s.encodeTags(enc, tagsIter) - if err != nil { // This is an invariant, should never happen - s.metrics.fetchTagged.ReportError(s.nowFn().Sub(callStart)) - return nil, tterrors.NewInternalError(err) - } - elem := &rpc.FetchTaggedIDResult_{ - NameSpace: nsID.Bytes(), - ID: tsID.Bytes(), - EncodedTags: encodedTags.Bytes(), - } - response.Elements = append(response.Elements, elem) - if !fetchData { - continue - } - segments, rpcErr := s.readEncoded(ctx, nsID, tsID, opts.StartInclusive, opts.EndExclusive) - if rpcErr != nil { - elem.Err = rpcErr - continue + var encodeErr error + results.WithMap(func(rMap *index.ResultsMap) { + for _, entry := range rMap.Iter() { + tsID := entry.Key() + tags := entry.Value() + enc := s.pools.tagEncoder.Get() + ctx.RegisterFinalizer(enc) + tagsIter.Reset(tags) + encodedTags, err := s.encodeTags(enc, tagsIter) + if err != nil { + encodeErr = err + return + } + + elem := &rpc.FetchTaggedIDResult_{ + NameSpace: nsID.Bytes(), + ID: tsID.Bytes(), + EncodedTags: encodedTags.Bytes(), + } + response.Elements = append(response.Elements, elem) + if !fetchData { + continue + } + segments, rpcErr := s.readEncoded(ctx, nsID, tsID, opts.StartInclusive, opts.EndExclusive) + if rpcErr != nil { + elem.Err = rpcErr + continue + } + elem.Segments = segments } - elem.Segments = segments + }) + if encodeErr != nil { + s.metrics.fetchTagged.ReportError(s.nowFn().Sub(callStart)) + return nil, tterrors.NewInternalError(err) } s.metrics.fetchTagged.ReportSuccess(s.nowFn().Sub(callStart)) diff --git a/src/dbnode/network/server/tchannelthrift/node/service_test.go b/src/dbnode/network/server/tchannelthrift/node/service_test.go index acdc0cd8a7..88ac1c1cec 100644 --- a/src/dbnode/network/server/tchannelthrift/node/service_test.go +++ b/src/dbnode/network/server/tchannelthrift/node/service_test.go @@ -193,14 +193,16 @@ func TestServiceQuery(t *testing.T) { resMap := index.NewResults(testIndexOptions) resMap.Reset(ident.StringID(nsID)) - resMap.Map().Set(ident.StringID("foo"), ident.NewTags( - ident.StringTag(tags["foo"][0].name, tags["foo"][0].value), - ident.StringTag(tags["foo"][1].name, tags["foo"][1].value), - )) - resMap.Map().Set(ident.StringID("bar"), ident.NewTags( - ident.StringTag(tags["bar"][0].name, tags["bar"][0].value), - ident.StringTag(tags["bar"][1].name, tags["bar"][1].value), - )) + resMap.WithMap(func(rMap *index.ResultsMap) { + rMap.Set(ident.StringID("foo"), ident.NewTags( + ident.StringTag(tags["foo"][0].name, tags["foo"][0].value), + ident.StringTag(tags["foo"][1].name, tags["foo"][1].value), + )) + rMap.Set(ident.StringID("bar"), ident.NewTags( + ident.StringTag(tags["bar"][0].name, tags["bar"][0].value), + ident.StringTag(tags["bar"][1].name, tags["bar"][1].value), + )) + }) mockDB.EXPECT().QueryIDs( ctx, @@ -1066,14 +1068,16 @@ func TestServiceFetchTagged(t *testing.T) { resMap := index.NewResults(testIndexOptions) resMap.Reset(ident.StringID(nsID)) - resMap.Map().Set(ident.StringID("foo"), ident.NewTags( - ident.StringTag("foo", "bar"), - ident.StringTag("baz", "dxk"), - )) - resMap.Map().Set(ident.StringID("bar"), ident.NewTags( - ident.StringTag("foo", "bar"), - ident.StringTag("dzk", "baz"), - )) + resMap.WithMap(func(rMap *index.ResultsMap) { + rMap.Set(ident.StringID("foo"), ident.NewTags( + ident.StringTag("foo", "bar"), + ident.StringTag("baz", "dxk"), + )) + rMap.Set(ident.StringID("bar"), ident.NewTags( + ident.StringTag("foo", "bar"), + ident.StringTag("dzk", "baz"), + )) + }) mockDB.EXPECT().QueryIDs( ctx, @@ -1163,14 +1167,16 @@ func TestServiceFetchTaggedIsOverloaded(t *testing.T) { resMap := index.NewResults(testIndexOptions) resMap.Reset(ident.StringID(nsID)) - resMap.Map().Set(ident.StringID("foo"), ident.NewTags( - ident.StringTag("foo", "bar"), - ident.StringTag("baz", "dxk"), - )) - resMap.Map().Set(ident.StringID("bar"), ident.NewTags( - ident.StringTag("foo", "bar"), - ident.StringTag("dzk", "baz"), - )) + resMap.WithMap(func(rMap *index.ResultsMap) { + rMap.Set(ident.StringID("foo"), ident.NewTags( + ident.StringTag("foo", "bar"), + ident.StringTag("baz", "dxk"), + )) + rMap.Set(ident.StringID("bar"), ident.NewTags( + ident.StringTag("foo", "bar"), + ident.StringTag("dzk", "baz"), + )) + }) startNanos, err := convert.ToValue(start, rpc.TimeType_UNIX_NANOSECONDS) require.NoError(t, err) @@ -1216,8 +1222,11 @@ func TestServiceFetchTaggedNoData(t *testing.T) { resMap := index.NewResults(testIndexOptions) resMap.Reset(ident.StringID(nsID)) - resMap.Map().Set(ident.StringID("foo"), ident.Tags{}) - resMap.Map().Set(ident.StringID("bar"), ident.Tags{}) + resMap.WithMap(func(rMap *index.ResultsMap) { + rMap.Set(ident.StringID("foo"), ident.Tags{}) + rMap.Set(ident.StringID("bar"), ident.Tags{}) + }) + mockDB.EXPECT().QueryIDs( ctx, ident.NewIDMatcher(nsID), diff --git a/src/dbnode/storage/index.go b/src/dbnode/storage/index.go index 83298375b8..24666123f5 100644 --- a/src/dbnode/storage/index.go +++ b/src/dbnode/storage/index.go @@ -895,24 +895,16 @@ func (i *nsIndex) Query( exhaustive bool returned bool }{ - merged: nil, + merged: i.resultsPool.Get(), exhaustive: true, returned: false, } ) - defer func() { - // Ensure that during early error returns we let any aborted - // goroutines know not to try to modify/edit the result any longer. - results.Lock() - results.returned = true - results.Unlock() - }() + // Set the results namespace ID + results.merged.Reset(i.nsMetadata.ID()) execBlockQuery := func(block index.Block) { - blockResults := i.resultsPool.Get() - blockResults.Reset(i.nsMetadata.ID()) - - blockExhaustive, err := block.Query(query, opts, blockResults) + blockExhaustive, err := block.Query(query, opts, results.merged) if err == index.ErrUnableToQueryBlockClosed { // NB(r): Because we query this block outside of the results lock, it's // possible this block may get closed if it slides out of retention, in @@ -921,53 +913,14 @@ func (i *nsIndex) Query( err = nil } - var mergedResult bool results.Lock() - defer func() { - results.Unlock() - if mergedResult { - // Only finalize this result if we merged it into another. - blockResults.Finalize() - } - }() - - if results.returned { - // If already returned then we early cancelled, don't add any - // further results or errors since caller already has a result. - return - } + defer results.Unlock() if err != nil { results.multiErr = results.multiErr.Add(err) return } - if results.merged == nil { - // Return results to pool at end of request. - ctx.RegisterFinalizer(blockResults) - // No merged results yet, use this as the first to merge into. - results.merged = blockResults - } else { - // Append the block results. - mergedResult = true - size := results.merged.Size() - for _, entry := range blockResults.Map().Iter() { - // Break early if reached limit. - if opts.Limit > 0 && size >= opts.Limit { - blockExhaustive = false - break - } - - // Append to merged results. - id, tags := entry.Key(), entry.Value() - _, size, err = results.merged.AddIDAndTags(id, tags) - if err != nil { - results.multiErr = results.multiErr.Add(err) - return - } - } - } - // If block had more data but we stopped early, need to notify caller. if blockExhaustive { return @@ -1064,8 +1017,6 @@ func (i *nsIndex) Query( } results.Lock() - // Signal not to add any further results since we've returned already. - results.returned = true // Take reference to vars to return while locked, need to allow defer // lock/unlock cleanup to not deadlock with this locked code block. exhaustive := results.exhaustive @@ -1077,12 +1028,6 @@ func (i *nsIndex) Query( return index.QueryResults{}, err } - // If no blocks queried, return an empty result - if mergedResults == nil { - mergedResults = i.resultsPool.Get() - mergedResults.Reset(i.nsMetadata.ID()) - } - return index.QueryResults{ Exhaustive: exhaustive, Results: mergedResults, diff --git a/src/dbnode/storage/index/block.go b/src/dbnode/storage/index/block.go index 6465cfcc89..49ebf45f07 100644 --- a/src/dbnode/storage/index/block.go +++ b/src/dbnode/storage/index/block.go @@ -73,6 +73,8 @@ const ( blockStateSealed blockStateClosed + defaultQueryDocsBatchSize = 256 + compactDebugLogEvery = 1 // Emit debug log for every compaction ) @@ -772,23 +774,50 @@ func (b *block) Query( } size := results.Size() - limitedResults := false + batch := b.docsPool.Get() + batchSize := cap(batch) + if batchSize == 0 { + batchSize = defaultQueryDocsBatchSize + } + batchOpts := AddDocumentsBatchResultsOptions{ + AllowPartialUpdates: true, + LimitSize: opts.Limit, + } iterCloser := safeCloser{closable: iter} execCloser := safeCloser{closable: exec} defer func() { + b.docsPool.Put(batch) iterCloser.Close() execCloser.Close() }() for iter.Next() { if opts.LimitExceeded(size) { - limitedResults = true break } - d := iter.Current() - _, size, err = results.AddDocument(d) + batch = append(batch, iter.Current()) + if len(batch) < batchSize { + continue + } + + _, size, err = results.AddDocumentsBatch(batch, batchOpts) + if err != nil { + return false, err + } + + // Reset batch + var emptyDoc doc.Document + for i := range batch { + batch[i] = emptyDoc + } + batch = batch[:0] + } + + // Put last batch if remainding + if len(batch) > 0 { + _, size, err = results.AddDocumentsBatch(batch, batchOpts) if err != nil { return false, err } @@ -806,7 +835,7 @@ func (b *block) Query( return false, err } - exhaustive := !limitedResults + exhaustive := !opts.LimitExceeded(size) return exhaustive, nil } diff --git a/src/dbnode/storage/index/block_prop_test.go b/src/dbnode/storage/index/block_prop_test.go index 3244ea4bf5..d95835ebbe 100644 --- a/src/dbnode/storage/index/block_prop_test.go +++ b/src/dbnode/storage/index/block_prop_test.go @@ -125,20 +125,27 @@ func TestPostingsListCacheDoesNotAffectBlockQueryResults(t *testing.T) { return false, errors.New("querying cached block was not exhaustive") } - uncachedMap := uncachedResults.Map() - cachedMap := cachedResults.Map() - if uncachedMap.Len() != cachedMap.Len() { + if uncachedResults.Size() != cachedResults.Size() { return false, fmt.Errorf( "uncached map size was: %d, but cached map sized was: %d", - uncachedMap.Len(), cachedMap.Len()) + uncachedResults.Size(), cachedResults.Size()) } - for _, entry := range uncachedMap.Iter() { - key := entry.Key() - _, ok := cachedMap.Get(key) - if !ok { - return false, fmt.Errorf("cached map did not contain: %v", key) - } + var checkMapsErr error + uncachedResults.WithMap(func(uncachedMap *ResultsMap) { + cachedResults.WithMap(func(cachedMap *ResultsMap) { + for _, entry := range uncachedMap.Iter() { + key := entry.Key() + _, ok := cachedMap.Get(key) + if !ok { + checkMapsErr = fmt.Errorf("cached map did not contain: %v", key) + return + } + } + }) + }) + if checkMapsErr != nil { + return false, checkMapsErr } } diff --git a/src/dbnode/storage/index/block_test.go b/src/dbnode/storage/index/block_test.go index 14a6facf2b..a9b965e328 100644 --- a/src/dbnode/storage/index/block_test.go +++ b/src/dbnode/storage/index/block_test.go @@ -510,13 +510,14 @@ func TestBlockMockQueryExecutorExecLimit(t *testing.T) { require.NoError(t, err) require.False(t, exhaustive) - rMap := results.Map() - require.Equal(t, 1, rMap.Len()) - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) + results.WithMap(func(rMap *ResultsMap) { + require.Equal(t, 1, rMap.Len()) + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) + }) } func TestBlockMockQueryExecutorExecIterCloseErr(t *testing.T) { @@ -611,13 +612,14 @@ func TestBlockMockQueryLimit(t *testing.T) { require.NoError(t, err) require.False(t, exhaustive) - rMap := results.Map() - require.Equal(t, 1, rMap.Len()) - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) + results.WithMap(func(rMap *ResultsMap) { + require.Equal(t, 1, rMap.Len()) + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) + }) } func TestBlockMockQueryLimitExhaustive(t *testing.T) { @@ -652,13 +654,14 @@ func TestBlockMockQueryLimitExhaustive(t *testing.T) { require.NoError(t, err) require.True(t, exhaustive) - rMap := results.Map() - require.Equal(t, 1, rMap.Len()) - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) + results.WithMap(func(rMap *ResultsMap) { + require.Equal(t, 1, rMap.Len()) + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) + }) } func TestBlockMockQueryMergeResultsMapLimit(t *testing.T) { @@ -695,13 +698,14 @@ func TestBlockMockQueryMergeResultsMapLimit(t *testing.T) { require.NoError(t, err) require.False(t, exhaustive) - rMap := results.Map() - require.Equal(t, 1, rMap.Len()) - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) + results.WithMap(func(rMap *ResultsMap) { + require.Equal(t, 1, rMap.Len()) + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) + }) } func TestBlockMockQueryMergeResultsDupeID(t *testing.T) { @@ -741,19 +745,20 @@ func TestBlockMockQueryMergeResultsDupeID(t *testing.T) { require.NoError(t, err) require.True(t, exhaustive) - rMap := results.Map() - require.Equal(t, 2, rMap.Len()) - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) + results.WithMap(func(rMap *ResultsMap) { + require.Equal(t, 2, rMap.Len()) + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) - t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( - ident.NewTagsIterator(t2))) + t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( + ident.NewTagsIterator(t2))) + }) } func TestBlockAddResultsAddsSegment(t *testing.T) { @@ -1143,18 +1148,19 @@ func TestBlockE2EInsertQuery(t *testing.T) { require.True(t, exhaustive) require.Equal(t, 2, results.Size()) - rMap := results.Map() - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) + results.WithMap(func(rMap *ResultsMap) { + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) - t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( - ident.NewTagsIterator(t2))) + t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( + ident.NewTagsIterator(t2))) + }) } func TestBlockE2EInsertQueryLimit(t *testing.T) { @@ -1209,25 +1215,26 @@ func TestBlockE2EInsertQueryLimit(t *testing.T) { require.False(t, exhaustive) require.Equal(t, 1, results.Size()) - rMap := results.Map() - numFound := 0 - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - if ok { - numFound++ - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) - } + results.WithMap(func(rMap *ResultsMap) { + numFound := 0 + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + if ok { + numFound++ + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) + } - t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) - if ok { - numFound++ - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( - ident.NewTagsIterator(t2))) - } + t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) + if ok { + numFound++ + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( + ident.NewTagsIterator(t2))) + } - require.Equal(t, 1, numFound) + require.Equal(t, 1, numFound) + }) } func TestBlockE2EInsertAddResultsQuery(t *testing.T) { @@ -1287,18 +1294,19 @@ func TestBlockE2EInsertAddResultsQuery(t *testing.T) { require.True(t, exhaustive) require.Equal(t, 2, results.Size()) - rMap := results.Map() - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) + results.WithMap(func(rMap *ResultsMap) { + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) - t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( - ident.NewTagsIterator(t2))) + t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( + ident.NewTagsIterator(t2))) + }) } func TestBlockE2EInsertAddResultsMergeQuery(t *testing.T) { @@ -1350,18 +1358,19 @@ func TestBlockE2EInsertAddResultsMergeQuery(t *testing.T) { require.True(t, exhaustive) require.Equal(t, 2, results.Size()) - rMap := results.Map() - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) + results.WithMap(func(rMap *ResultsMap) { + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) - t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( - ident.NewTagsIterator(t2))) + t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( + ident.NewTagsIterator(t2))) + }) } func TestBlockWriteBackgroundCompact(t *testing.T) { diff --git a/src/dbnode/storage/index/results.go b/src/dbnode/storage/index/results.go index b09ffe5a14..150d2c3704 100644 --- a/src/dbnode/storage/index/results.go +++ b/src/dbnode/storage/index/results.go @@ -22,6 +22,7 @@ package index import ( "errors" + "sync" "github.com/m3db/m3/src/m3ninx/doc" "github.com/m3db/m3x/ident" @@ -29,10 +30,12 @@ import ( ) var ( - errUnableToAddResultMissingID = errors.New("no id for result") + errUnableToAddResultMissingID = errors.New("no id for result") + errResultAlreadyExistsNoPartialUpdates = errors.New("id already exists for result and partial updates not allowed") ) type results struct { + sync.RWMutex nsID ident.ID resultsMap *ResultsMap @@ -55,6 +58,15 @@ func NewResults(opts Options) Results { func (r *results) AddDocument( d doc.Document, +) (added bool, size int, err error) { + r.Lock() + added, size, err = r.addDocumentWithLock(d) + r.Unlock() + return +} + +func (r *results) addDocumentWithLock( + d doc.Document, ) (added bool, size int, err error) { added = false if len(d.ID) == 0 { @@ -82,9 +94,55 @@ func (r *results) AddDocument( return added, r.resultsMap.Len(), nil } +func (r *results) AddDocumentsBatch( + batch []doc.Document, + opts AddDocumentsBatchResultsOptions, +) (numPartialUpdates int, size int, err error) { + r.Lock() + numPartialUpdates, size, err = r.addDocumentsBatchWithLock(batch, opts) + r.Unlock() + return +} + +func (r *results) addDocumentsBatchWithLock( + batch []doc.Document, + opts AddDocumentsBatchResultsOptions, +) (numPartialUpdates int, size int, err error) { + numPartialUpdates = 0 + size = r.resultsMap.Len() + for i := range batch { + var added bool + added, size, err = r.addDocumentWithLock(batch[i]) + if err != nil { + return numPartialUpdates, size, err + } + if !added { + if !opts.AllowPartialUpdates { + return numPartialUpdates, size, errResultAlreadyExistsNoPartialUpdates + } + numPartialUpdates++ + } + if opts.LimitSize > 0 && size >= opts.LimitSize { + // Early return if limit enforced and we hit our limit + break + } + } + return numPartialUpdates, size, nil +} + func (r *results) AddIDAndTags( id ident.ID, tags ident.Tags, +) (added bool, size int, err error) { + r.Lock() + added, size, err = r.addIDAndTagsWithLock(id, tags) + r.Unlock() + return +} + +func (r *results) addIDAndTagsWithLock( + id ident.ID, + tags ident.Tags, ) (added bool, size int, err error) { added = false bytesID := ident.BytesID(id.Bytes()) @@ -121,18 +179,29 @@ func (r *results) cloneTagsFromFields(fields doc.Fields) ident.Tags { } func (r *results) Namespace() ident.ID { - return r.nsID + r.RLock() + v := r.nsID + r.RUnlock() + return v } -func (r *results) Map() *ResultsMap { - return r.resultsMap +func (r *results) WithMap(fn func(results *ResultsMap)) { + r.RLock() + fn(r.resultsMap) + r.RUnlock() } func (r *results) Size() int { - return r.resultsMap.Len() + r.RLock() + v := r.resultsMap.Len() + r.RUnlock() + return v } func (r *results) Reset(nsID ident.ID) { + r.Lock() + defer r.Unlock() + // finalize existing held nsID if r.nsID != nil { r.nsID.Finalize() @@ -157,7 +226,11 @@ func (r *results) Reset(nsID ident.ID) { } func (r *results) Finalize() { - if r.noFinalize { + r.RLock() + noFinalize := r.noFinalize + r.RUnlock() + + if noFinalize { return } @@ -170,6 +243,9 @@ func (r *results) Finalize() { } func (r *results) NoFinalize() { + r.Lock() + defer r.Unlock() + // Ensure neither the results object itself, or any of its underlying // IDs and tags will be finalized. r.noFinalize = true diff --git a/src/dbnode/storage/index/results_test.go b/src/dbnode/storage/index/results_test.go index a2ffb0b563..000582b7aa 100644 --- a/src/dbnode/storage/index/results_test.go +++ b/src/dbnode/storage/index/results_test.go @@ -71,9 +71,11 @@ func TestResultsFirstInsertWins(t *testing.T) { require.True(t, added) require.Equal(t, 1, size) - tags, ok := res.Map().Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) + res.WithMap(func(rMap *ResultsMap) { + tags, ok := rMap.Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) + }) d2 := doc.Document{ID: []byte("abc"), Fields: doc.Fields{ @@ -84,9 +86,11 @@ func TestResultsFirstInsertWins(t *testing.T) { require.False(t, added) require.Equal(t, 1, size) - tags, ok = res.Map().Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) + res.WithMap(func(rMap *ResultsMap) { + tags, ok := rMap.Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) + }) } func TestResultsInsertContains(t *testing.T) { @@ -97,9 +101,11 @@ func TestResultsInsertContains(t *testing.T) { require.True(t, added) require.Equal(t, 1, size) - tags, ok := res.Map().Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) + res.WithMap(func(rMap *ResultsMap) { + tags, ok := rMap.Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) + }) } func TestResultsInsertCopies(t *testing.T) { @@ -116,33 +122,35 @@ func TestResultsInsertCopies(t *testing.T) { // our genny generated maps don't provide access to MapEntry directly, // so we iterate over the map to find the added entry. Could avoid this // in the future if we expose `func (m *Map) Entry(k Key) Entry {}` - for _, entry := range res.Map().Iter() { - // see if this key has the same value as the added document's ID. - key := entry.Key().Bytes() - if !bytes.Equal(dValid.ID, key) { - continue - } - found = true - // ensure the underlying []byte for ID/Fields is at a different address - // than the original. - require.False(t, xtest.ByteSlicesBackedBySameData(key, dValid.ID)) - tags := entry.Value().Values() - for _, f := range dValid.Fields { - fName := f.Name - fValue := f.Value - for _, tag := range tags { - tName := tag.Name.Bytes() - tValue := tag.Value.Bytes() - if !bytes.Equal(fName, tName) || !bytes.Equal(fValue, tValue) { - continue + res.WithMap(func(rMap *ResultsMap) { + for _, entry := range rMap.Iter() { + // see if this key has the same value as the added document's ID. + key := entry.Key().Bytes() + if !bytes.Equal(dValid.ID, key) { + continue + } + found = true + // ensure the underlying []byte for ID/Fields is at a different address + // than the original. + require.False(t, xtest.ByteSlicesBackedBySameData(key, dValid.ID)) + tags := entry.Value().Values() + for _, f := range dValid.Fields { + fName := f.Name + fValue := f.Value + for _, tag := range tags { + tName := tag.Name.Bytes() + tValue := tag.Value.Bytes() + if !bytes.Equal(fName, tName) || !bytes.Equal(fValue, tValue) { + continue + } + require.False(t, xtest.ByteSlicesBackedBySameData(fName, tName)) + require.False(t, xtest.ByteSlicesBackedBySameData(fValue, tValue)) } - require.False(t, xtest.ByteSlicesBackedBySameData(fName, tName)) - require.False(t, xtest.ByteSlicesBackedBySameData(fValue, tValue)) } } - } - require.True(t, found) + require.True(t, found) + }) } func TestResultsInsertIDAndTagsInvalid(t *testing.T) { @@ -175,9 +183,11 @@ func TestResultsIDAndTagsFirstInsertWins(t *testing.T) { require.True(t, added) require.Equal(t, 1, size) - tags, ok := res.Map().Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) + res.WithMap(func(rMap *ResultsMap) { + tags, ok := rMap.Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) + }) d2 := doc.Document{ID: []byte("abc"), Fields: doc.Fields{ @@ -188,9 +198,11 @@ func TestResultsIDAndTagsFirstInsertWins(t *testing.T) { require.False(t, added) require.Equal(t, 1, size) - tags, ok = res.Map().Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) + res.WithMap(func(rMap *ResultsMap) { + tags, ok := rMap.Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) + }) } func TestResultsReset(t *testing.T) { @@ -201,15 +213,20 @@ func TestResultsReset(t *testing.T) { require.True(t, added) require.Equal(t, 1, size) - tags, ok := res.Map().Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) + res.WithMap(func(rMap *ResultsMap) { + tags, ok := rMap.Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) + }) res.Reset(nil) - _, ok = res.Map().Get(ident.StringID("abc")) - require.False(t, ok) - require.Equal(t, 0, len(tags.Values())) - require.Equal(t, 0, res.Size()) + + res.WithMap(func(rMap *ResultsMap) { + tags, ok := rMap.Get(ident.StringID("abc")) + require.False(t, ok) + require.Equal(t, 0, len(tags.Values())) + require.Equal(t, 0, res.Size()) + }) } func TestResultsResetNamespaceClones(t *testing.T) { @@ -242,25 +259,29 @@ func TestFinalize(t *testing.T) { require.Equal(t, 1, size) // Ensure the data is present. - tags, ok := res.Map().Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) + res.WithMap(func(rMap *ResultsMap) { + tags, ok := rMap.Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) + }) // Call Finalize() to reset the Results. res.Finalize() // Ensure data was removed by call to Finalize(). - tags, ok = res.Map().Get(ident.StringID("abc")) - require.False(t, ok) - require.Equal(t, 0, len(tags.Values())) - require.Equal(t, 0, res.Size()) - - for _, entry := range res.Map().Iter() { - id, _ := entry.Key(), entry.Value() - require.False(t, id.IsNoFinalize()) - // TODO(rartoul): Could verify tags are NoFinalize() as well if - // they had that method. - } + res.WithMap(func(rMap *ResultsMap) { + tags, ok := rMap.Get(ident.StringID("abc")) + require.False(t, ok) + require.Equal(t, 0, len(tags.Values())) + require.Equal(t, 0, res.Size()) + + for _, entry := range rMap.Iter() { + id, _ := entry.Key(), entry.Value() + require.False(t, id.IsNoFinalize()) + // TODO(rartoul): Could verify tags are NoFinalize() as well if + // they had that method. + } + }) } func TestNoFinalize(t *testing.T) { @@ -273,9 +294,11 @@ func TestNoFinalize(t *testing.T) { require.Equal(t, 1, size) // Ensure the data is present. - tags, ok := res.Map().Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) + res.WithMap(func(rMap *ResultsMap) { + tags, ok := rMap.Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) + }) // Call to NoFinalize indicates that subsequent call // to finalize should be a no-op. @@ -283,15 +306,17 @@ func TestNoFinalize(t *testing.T) { res.Finalize() // Ensure data was not removed by call to Finalize(). - tags, ok = res.Map().Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) - require.Equal(t, 1, res.Size()) - - for _, entry := range res.Map().Iter() { - id, _ := entry.Key(), entry.Value() - require.True(t, id.IsNoFinalize()) - // TODO(rartoul): Could verify tags are NoFinalize() as well if - // they had that method. - } + res.WithMap(func(rMap *ResultsMap) { + tags, ok := rMap.Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) + require.Equal(t, 1, res.Size()) + + for _, entry := range rMap.Iter() { + id, _ := entry.Key(), entry.Value() + require.True(t, id.IsNoFinalize()) + // TODO(rartoul): Could verify tags are NoFinalize() as well if + // they had that method. + } + }) } diff --git a/src/dbnode/storage/index/types.go b/src/dbnode/storage/index/types.go index 6397181fe5..87bb14e091 100644 --- a/src/dbnode/storage/index/types.go +++ b/src/dbnode/storage/index/types.go @@ -79,13 +79,17 @@ type QueryResults struct { Exhaustive bool } -// Results is a collection of results for a query. +// Results is a collection of results for a query, it is synchronized +// when access to the results set is used as documented by the methods. type Results interface { // Namespace returns the namespace associated with the result. Namespace() ident.ID - // Map returns a map from seriesID -> seriesTags, comprising index results. - Map() *ResultsMap + // WithMap executes a function that has access to a results map + // from seriesID -> seriesTags, comprising index results. + // A function is required to ensure that the results is used + // while a read lock for the results is held. + WithMap(fn func(results *ResultsMap)) // Reset resets the Results object to initial state. Reset(nsID ident.ID) @@ -111,6 +115,14 @@ type Results interface { document doc.Document, ) (added bool, size int, err error) + // AddDocumentsBatch adds the batch of documents to the results set, it will + // take a copy of the bytes backing the documents so the original can be modified + // after this function returns without affecting the results map. + AddDocumentsBatch( + batch []doc.Document, + opts AddDocumentsBatchResultsOptions, + ) (numPartialUpdates int, size int, err error) + // AddIDAndTagsOrFinalize adds ID and tags to the results and takes ownership // if it does not exist, otherwise if it's already contained it returns added // false. @@ -120,6 +132,22 @@ type Results interface { ) (added bool, size int, err error) } +// AddDocumentsBatchResultsOptions is a set of options to use when adding +// results for a query. +type AddDocumentsBatchResultsOptions struct { + // If AllowPartialUpdates is true the query result will continue to add results + // even if it encounters an error attempting to add document that already exists + // in the results set. + // If false, on the other hand, then any errors encountered adding a document + // that already exists to the results set will cause the addition of a batch to + // fail and none of the documents in the batch will be added to the results. + AllowPartialUpdates bool + + // LimitSize will limit the total results set to a given limit and if overflown + // will return early successfully. + LimitSize int +} + // ResultsAllocator allocates Results types. type ResultsAllocator func() Results diff --git a/src/dbnode/storage/index_query_concurrent_test.go b/src/dbnode/storage/index_query_concurrent_test.go index b597018866..97b52cc4e8 100644 --- a/src/dbnode/storage/index_query_concurrent_test.go +++ b/src/dbnode/storage/index_query_concurrent_test.go @@ -313,26 +313,29 @@ func testNamespaceIndexHighConcurrentQueries( // Read the results concurrently too hits := make(map[string]struct{}, results.Results.Size()) - for _, entry := range results.Results.Map().Iter() { - id := entry.Key().String() - - doc, err := convert.FromMetricNoClone(entry.Key(), entry.Value()) - require.NoError(t, err) - if err != nil { - continue // this will fail the test anyway, but don't want to panic - } - - expectedDoc, ok := expectedResults[id] - require.True(t, ok) - if !ok { - continue // this will fail the test anyway, but don't want to panic + results.Results.WithMap(func(rMap *index.ResultsMap) { + for _, entry := range rMap.Iter() { + id := entry.Key().String() + + doc, err := convert.FromMetricNoClone(entry.Key(), entry.Value()) + require.NoError(t, err) + if err != nil { + continue // this will fail the test anyway, but don't want to panic + } + + expectedDoc, ok := expectedResults[id] + require.True(t, ok) + if !ok { + continue // this will fail the test anyway, but don't want to panic + } + + require.Equal(t, expectedDoc, doc) + hits[id] = struct{}{} } - require.Equal(t, expectedDoc, doc) - hits[id] = struct{}{} - } - expectedHits := idsPerBlock * (k + 1) - require.Equal(t, expectedHits, len(hits)) + expectedHits := idsPerBlock * (k + 1) + require.Equal(t, expectedHits, len(hits)) + }) } }() } diff --git a/src/dbnode/storage/index_queue_test.go b/src/dbnode/storage/index_queue_test.go index 7001086e8b..9c05b15ac9 100644 --- a/src/dbnode/storage/index_queue_test.go +++ b/src/dbnode/storage/index_queue_test.go @@ -316,9 +316,11 @@ func TestNamespaceIndexInsertQuery(t *testing.T) { assert.True(t, res.Exhaustive) results := res.Results assert.Equal(t, "testns1", results.Namespace().String()) - tags, ok := results.Map().Get(ident.StringID("foo")) - assert.True(t, ok) - assert.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("name", "value")).Matches( - ident.NewTagsIterator(tags))) + results.WithMap(func(rMap *index.ResultsMap) { + tags, ok := rMap.Get(ident.StringID("foo")) + assert.True(t, ok) + assert.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("name", "value")).Matches( + ident.NewTagsIterator(tags))) + }) } From 760b41f3e5d44da82894e201822f9fce84c76a03 Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Wed, 20 Mar 2019 23:18:14 -0400 Subject: [PATCH 02/12] Use cancellable lifetimes to defend against altering state after index query returns due timeout or other error --- .../integration/write_tagged_quorum_test.go | 10 +- .../server/tchannelthrift/node/service.go | 112 +++---- .../tchannelthrift/node/service_test.go | 61 ++-- src/dbnode/storage/index.go | 56 ++-- src/dbnode/storage/index/block.go | 47 ++- src/dbnode/storage/index/block_prop_test.go | 31 +- src/dbnode/storage/index/block_test.go | 273 +++++++++--------- src/dbnode/storage/index/index_mock.go | 52 ++-- src/dbnode/storage/index/options.go | 2 +- src/dbnode/storage/index/results.go | 171 ++++------- src/dbnode/storage/index/results_test.go | 270 +++++++---------- src/dbnode/storage/index/types.go | 81 ++---- src/dbnode/storage/index_block_test.go | 8 +- .../storage/index_query_concurrent_test.go | 60 ++-- src/dbnode/storage/index_queue_test.go | 12 +- src/x/resource/lifetime.go | 67 +++++ src/x/resource/lifetime_test.go | 69 +++++ 17 files changed, 679 insertions(+), 703 deletions(-) create mode 100644 src/x/resource/lifetime.go create mode 100644 src/x/resource/lifetime_test.go diff --git a/src/dbnode/integration/write_tagged_quorum_test.go b/src/dbnode/integration/write_tagged_quorum_test.go index 62d4476116..ca83a9e660 100644 --- a/src/dbnode/integration/write_tagged_quorum_test.go +++ b/src/dbnode/integration/write_tagged_quorum_test.go @@ -300,13 +300,9 @@ func nodeHasTaggedWrite(t *testing.T, s *testSetup) bool { require.NoError(t, err) results := res.Results require.Equal(t, testNamespaces[0].String(), results.Namespace().String()) - - var idxFound bool - results.WithMap(func(rMap *index.ResultsMap) { - tags, ok := rMap.Get(ident.StringID("quorumTest")) - idxFound = ok && ident.NewTagIterMatcher(ident.MustNewTagStringsIterator( - "foo", "bar", "boo", "baz")).Matches(ident.NewTagsIterator(tags)) - }) + tags, ok := results.Map().Get(ident.StringID("quorumTest")) + idxFound := ok && ident.NewTagIterMatcher(ident.MustNewTagStringsIterator( + "foo", "bar", "boo", "baz")).Matches(ident.NewTagsIterator(tags)) if !idxFound { return false diff --git a/src/dbnode/network/server/tchannelthrift/node/service.go b/src/dbnode/network/server/tchannelthrift/node/service.go index c88fef5557..d303b77c70 100644 --- a/src/dbnode/network/server/tchannelthrift/node/service.go +++ b/src/dbnode/network/server/tchannelthrift/node/service.go @@ -289,45 +289,35 @@ func (s *service) Query(tctx thrift.Context, req *rpc.QueryRequest) (*rpc.QueryR } result := &rpc.QueryResult_{ - Results: make([]*rpc.QueryResultElement, 0, queryResult.Results.Size()), + Results: make([]*rpc.QueryResultElement, 0, queryResult.Results.Map().Len()), Exhaustive: queryResult.Exhaustive, } - fetchData := true if req.NoData != nil && *req.NoData { fetchData = false } - - var readErr error - queryResult.Results.WithMap(func(rMap *index.ResultsMap) { - for _, entry := range rMap.Iter() { - elem := &rpc.QueryResultElement{ - ID: entry.Key().String(), - Tags: make([]*rpc.Tag, 0, len(entry.Value().Values())), - } - result.Results = append(result.Results, elem) - for _, tag := range entry.Value().Values() { - elem.Tags = append(elem.Tags, &rpc.Tag{ - Name: tag.Name.String(), - Value: tag.Value.String(), - }) - } - if !fetchData { - continue - } - tsID := entry.Key() - datapoints, err := s.readDatapoints(ctx, nsID, tsID, start, end, - req.ResultTimeType) - if err != nil { - // Break for error - readErr = err - return - } - elem.Datapoints = datapoints + for _, entry := range queryResult.Results.Map().Iter() { + elem := &rpc.QueryResultElement{ + ID: entry.Key().String(), + Tags: make([]*rpc.Tag, 0, len(entry.Value().Values())), } - }) - if readErr != nil { - return nil, convert.ToRPCError(readErr) + result.Results = append(result.Results, elem) + for _, tag := range entry.Value().Values() { + elem.Tags = append(elem.Tags, &rpc.Tag{ + Name: tag.Name.String(), + Value: tag.Value.String(), + }) + } + if !fetchData { + continue + } + tsID := entry.Key() + datapoints, err := s.readDatapoints(ctx, nsID, tsID, start, end, + req.ResultTimeType) + if err != nil { + return nil, convert.ToRPCError(err) + } + elem.Datapoints = datapoints } return result, nil @@ -432,41 +422,33 @@ func (s *service) FetchTagged(tctx thrift.Context, req *rpc.FetchTaggedRequest) results := queryResult.Results nsID := results.Namespace() tagsIter := ident.NewTagsIterator(ident.Tags{}) + for _, entry := range results.Map().Iter() { + tsID := entry.Key() + tags := entry.Value() + enc := s.pools.tagEncoder.Get() + ctx.RegisterFinalizer(enc) + tagsIter.Reset(tags) + encodedTags, err := s.encodeTags(enc, tagsIter) + if err != nil { // This is an invariant, should never happen + s.metrics.fetchTagged.ReportError(s.nowFn().Sub(callStart)) + return nil, tterrors.NewInternalError(err) + } - var encodeErr error - results.WithMap(func(rMap *index.ResultsMap) { - for _, entry := range rMap.Iter() { - tsID := entry.Key() - tags := entry.Value() - enc := s.pools.tagEncoder.Get() - ctx.RegisterFinalizer(enc) - tagsIter.Reset(tags) - encodedTags, err := s.encodeTags(enc, tagsIter) - if err != nil { - encodeErr = err - return - } - - elem := &rpc.FetchTaggedIDResult_{ - NameSpace: nsID.Bytes(), - ID: tsID.Bytes(), - EncodedTags: encodedTags.Bytes(), - } - response.Elements = append(response.Elements, elem) - if !fetchData { - continue - } - segments, rpcErr := s.readEncoded(ctx, nsID, tsID, opts.StartInclusive, opts.EndExclusive) - if rpcErr != nil { - elem.Err = rpcErr - continue - } - elem.Segments = segments + elem := &rpc.FetchTaggedIDResult_{ + NameSpace: nsID.Bytes(), + ID: tsID.Bytes(), + EncodedTags: encodedTags.Bytes(), } - }) - if encodeErr != nil { - s.metrics.fetchTagged.ReportError(s.nowFn().Sub(callStart)) - return nil, tterrors.NewInternalError(err) + response.Elements = append(response.Elements, elem) + if !fetchData { + continue + } + segments, rpcErr := s.readEncoded(ctx, nsID, tsID, opts.StartInclusive, opts.EndExclusive) + if rpcErr != nil { + elem.Err = rpcErr + continue + } + elem.Segments = segments } s.metrics.fetchTagged.ReportSuccess(s.nowFn().Sub(callStart)) diff --git a/src/dbnode/network/server/tchannelthrift/node/service_test.go b/src/dbnode/network/server/tchannelthrift/node/service_test.go index 88ac1c1cec..acdc0cd8a7 100644 --- a/src/dbnode/network/server/tchannelthrift/node/service_test.go +++ b/src/dbnode/network/server/tchannelthrift/node/service_test.go @@ -193,16 +193,14 @@ func TestServiceQuery(t *testing.T) { resMap := index.NewResults(testIndexOptions) resMap.Reset(ident.StringID(nsID)) - resMap.WithMap(func(rMap *index.ResultsMap) { - rMap.Set(ident.StringID("foo"), ident.NewTags( - ident.StringTag(tags["foo"][0].name, tags["foo"][0].value), - ident.StringTag(tags["foo"][1].name, tags["foo"][1].value), - )) - rMap.Set(ident.StringID("bar"), ident.NewTags( - ident.StringTag(tags["bar"][0].name, tags["bar"][0].value), - ident.StringTag(tags["bar"][1].name, tags["bar"][1].value), - )) - }) + resMap.Map().Set(ident.StringID("foo"), ident.NewTags( + ident.StringTag(tags["foo"][0].name, tags["foo"][0].value), + ident.StringTag(tags["foo"][1].name, tags["foo"][1].value), + )) + resMap.Map().Set(ident.StringID("bar"), ident.NewTags( + ident.StringTag(tags["bar"][0].name, tags["bar"][0].value), + ident.StringTag(tags["bar"][1].name, tags["bar"][1].value), + )) mockDB.EXPECT().QueryIDs( ctx, @@ -1068,16 +1066,14 @@ func TestServiceFetchTagged(t *testing.T) { resMap := index.NewResults(testIndexOptions) resMap.Reset(ident.StringID(nsID)) - resMap.WithMap(func(rMap *index.ResultsMap) { - rMap.Set(ident.StringID("foo"), ident.NewTags( - ident.StringTag("foo", "bar"), - ident.StringTag("baz", "dxk"), - )) - rMap.Set(ident.StringID("bar"), ident.NewTags( - ident.StringTag("foo", "bar"), - ident.StringTag("dzk", "baz"), - )) - }) + resMap.Map().Set(ident.StringID("foo"), ident.NewTags( + ident.StringTag("foo", "bar"), + ident.StringTag("baz", "dxk"), + )) + resMap.Map().Set(ident.StringID("bar"), ident.NewTags( + ident.StringTag("foo", "bar"), + ident.StringTag("dzk", "baz"), + )) mockDB.EXPECT().QueryIDs( ctx, @@ -1167,16 +1163,14 @@ func TestServiceFetchTaggedIsOverloaded(t *testing.T) { resMap := index.NewResults(testIndexOptions) resMap.Reset(ident.StringID(nsID)) - resMap.WithMap(func(rMap *index.ResultsMap) { - rMap.Set(ident.StringID("foo"), ident.NewTags( - ident.StringTag("foo", "bar"), - ident.StringTag("baz", "dxk"), - )) - rMap.Set(ident.StringID("bar"), ident.NewTags( - ident.StringTag("foo", "bar"), - ident.StringTag("dzk", "baz"), - )) - }) + resMap.Map().Set(ident.StringID("foo"), ident.NewTags( + ident.StringTag("foo", "bar"), + ident.StringTag("baz", "dxk"), + )) + resMap.Map().Set(ident.StringID("bar"), ident.NewTags( + ident.StringTag("foo", "bar"), + ident.StringTag("dzk", "baz"), + )) startNanos, err := convert.ToValue(start, rpc.TimeType_UNIX_NANOSECONDS) require.NoError(t, err) @@ -1222,11 +1216,8 @@ func TestServiceFetchTaggedNoData(t *testing.T) { resMap := index.NewResults(testIndexOptions) resMap.Reset(ident.StringID(nsID)) - resMap.WithMap(func(rMap *index.ResultsMap) { - rMap.Set(ident.StringID("foo"), ident.Tags{}) - rMap.Set(ident.StringID("bar"), ident.Tags{}) - }) - + resMap.Map().Set(ident.StringID("foo"), ident.Tags{}) + resMap.Map().Set(ident.StringID("bar"), ident.Tags{}) mockDB.EXPECT().QueryIDs( ctx, ident.NewIDMatcher(nsID), diff --git a/src/dbnode/storage/index.go b/src/dbnode/storage/index.go index 24666123f5..a4b1cc52b5 100644 --- a/src/dbnode/storage/index.go +++ b/src/dbnode/storage/index.go @@ -44,6 +44,7 @@ import ( m3ninxindex "github.com/m3db/m3/src/m3ninx/index" "github.com/m3db/m3/src/m3ninx/index/segment" "github.com/m3db/m3/src/m3ninx/index/segment/builder" + "github.com/m3db/m3/src/x/resource" xclose "github.com/m3db/m3x/close" "github.com/m3db/m3x/context" xerrors "github.com/m3db/m3x/errors" @@ -887,24 +888,29 @@ func (i *nsIndex) Query( deadline = start.Add(timeout) wg sync.WaitGroup - // Results contains all concurrent mutable state below. - results = struct { + // State contains concurrent mutable state for async execution below. + state = struct { sync.Mutex multiErr xerrors.MultiError - merged index.Results exhaustive bool - returned bool }{ - merged: i.resultsPool.Get(), exhaustive: true, - returned: false, } ) - // Set the results namespace ID - results.merged.Reset(i.nsMetadata.ID()) + + // Get results and set the namespace ID and size limit. + results := i.resultsPool.Get() + results.Reset(i.nsMetadata.ID(), index.ResultsOptions{ + SizeLimit: opts.Limit, + }) + + // Create a cancellable lifetime and cancel it at end of this method so that + // no child async task modifies the result after this method returns. + cancellable := resource.NewCancellableLifetime() + defer cancellable.Cancel() execBlockQuery := func(block index.Block) { - blockExhaustive, err := block.Query(query, opts, results.merged) + blockExhaustive, err := block.Query(cancellable, query, opts, results) if err == index.ErrUnableToQueryBlockClosed { // NB(r): Because we query this block outside of the results lock, it's // possible this block may get closed if it slides out of retention, in @@ -913,11 +919,11 @@ func (i *nsIndex) Query( err = nil } - results.Lock() - defer results.Unlock() + state.Lock() + defer state.Unlock() if err != nil { - results.multiErr = results.multiErr.Add(err) + state.multiErr = state.multiErr.Add(err) return } @@ -925,7 +931,7 @@ func (i *nsIndex) Query( if blockExhaustive { return } - results.exhaustive = false + state.exhaustive = false } for _, block := range blocks { @@ -933,16 +939,13 @@ func (i *nsIndex) Query( block := block // Terminate early if we know we don't need any more results. - results.Lock() - mergedSize := 0 - if results.merged != nil { - mergedSize = results.merged.Size() - } - alreadyNotExhaustive := opts.Limit > 0 && mergedSize >= opts.Limit + size := results.Size() + alreadyNotExhaustive := opts.LimitExceeded(size) if alreadyNotExhaustive { - results.exhaustive = false + state.Lock() + state.exhaustive = false + state.Unlock() } - results.Unlock() if alreadyNotExhaustive { // Break out if already exhaustive. @@ -1016,13 +1019,12 @@ func (i *nsIndex) Query( } } - results.Lock() + state.Lock() // Take reference to vars to return while locked, need to allow defer // lock/unlock cleanup to not deadlock with this locked code block. - exhaustive := results.exhaustive - mergedResults := results.merged - err = results.multiErr.FinalError() - results.Unlock() + exhaustive := state.exhaustive + err = state.multiErr.FinalError() + state.Unlock() if err != nil { return index.QueryResults{}, err @@ -1030,7 +1032,7 @@ func (i *nsIndex) Query( return index.QueryResults{ Exhaustive: exhaustive, - Results: mergedResults, + Results: results, }, nil } diff --git a/src/dbnode/storage/index/block.go b/src/dbnode/storage/index/block.go index 49ebf45f07..2408b63657 100644 --- a/src/dbnode/storage/index/block.go +++ b/src/dbnode/storage/index/block.go @@ -37,6 +37,7 @@ import ( "github.com/m3db/m3/src/m3ninx/index/segment/fst" "github.com/m3db/m3/src/m3ninx/search" "github.com/m3db/m3/src/m3ninx/search/executor" + "github.com/m3db/m3/src/x/resource" "github.com/m3db/m3x/context" xerrors "github.com/m3db/m3x/errors" "github.com/m3db/m3x/instrument" @@ -61,6 +62,7 @@ var ( errForegroundCompactorNoPlan = errors.New("index foreground compactor failed to generate a plan") errForegroundCompactorBadPlanFirstTask = errors.New("index foreground compactor generated plan without mutable segment in first task") errForegroundCompactorBadPlanSecondaryTask = errors.New("index foreground compactor generated plan with mutable segment a secondary task") + errCancelledQuery = errors.New("query was cancelled") errUnableToSealBlockIllegalStateFmtString = "unable to seal, index block state: %v" errUnableToWriteBlockUnknownStateFmtString = "unable to write, unknown index block state: %v" @@ -751,6 +753,7 @@ func (b *block) executorWithRLock() (search.Executor, error) { } func (b *block) Query( + cancellable *resource.CancellableLifetime, query Query, opts QueryOptions, results Results, @@ -779,10 +782,6 @@ func (b *block) Query( if batchSize == 0 { batchSize = defaultQueryDocsBatchSize } - batchOpts := AddDocumentsBatchResultsOptions{ - AllowPartialUpdates: true, - LimitSize: opts.Limit, - } iterCloser := safeCloser{closable: iter} execCloser := safeCloser{closable: exec} @@ -802,22 +801,15 @@ func (b *block) Query( continue } - _, size, err = results.AddDocumentsBatch(batch, batchOpts) + batch, size, err = b.addQueryResults(cancellable, results, batch) if err != nil { return false, err } - - // Reset batch - var emptyDoc doc.Document - for i := range batch { - batch[i] = emptyDoc - } - batch = batch[:0] } // Put last batch if remainding if len(batch) > 0 { - _, size, err = results.AddDocumentsBatch(batch, batchOpts) + batch, size, err = b.addQueryResults(cancellable, results, batch) if err != nil { return false, err } @@ -839,6 +831,35 @@ func (b *block) Query( return exhaustive, nil } +func (b *block) addQueryResults( + cancellable *resource.CancellableLifetime, + results Results, + batch []doc.Document, +) ([]doc.Document, int, error) { + // Checkout the lifetime of the query before adding results + queryValid := cancellable.TryCheckout() + if !queryValid { + // Query not valid any longer, do not add results and return early + return batch, 0, errCancelledQuery + } + + // Try to add the docs to the resource + size, err := results.AddDocuments(batch) + + // Immediately release the checkout on the lifetime of query + cancellable.ReleaseCheckout() + + // Reset batch + var emptyDoc doc.Document + for i := range batch { + batch[i] = emptyDoc + } + batch = batch[:0] + + // Return results + return batch, size, err +} + func (b *block) AddResults( results result.IndexBlock, ) error { diff --git a/src/dbnode/storage/index/block_prop_test.go b/src/dbnode/storage/index/block_prop_test.go index d95835ebbe..cb8d88e330 100644 --- a/src/dbnode/storage/index/block_prop_test.go +++ b/src/dbnode/storage/index/block_prop_test.go @@ -107,7 +107,7 @@ func TestPostingsListCacheDoesNotAffectBlockQueryResults(t *testing.T) { idx.NewQueryFromSearchQuery(q), } - uncachedResults := NewResults(testOpts) + uncachedResults := NewResults(ResultsOptions{}, testOpts) exhaustive, err := uncachedBlock.Query(indexQuery, QueryOptions{StartInclusive: blockStart, EndExclusive: blockStart.Add(blockSize)}, uncachedResults) if err != nil { return false, fmt.Errorf("error querying uncached block: %v", err) @@ -116,7 +116,7 @@ func TestPostingsListCacheDoesNotAffectBlockQueryResults(t *testing.T) { return false, errors.New("querying uncached block was not exhaustive") } - cachedResults := NewResults(testOpts) + cachedResults := NewResults(ResultsOptions{}, testOpts) exhaustive, err = cachedBlock.Query(indexQuery, QueryOptions{StartInclusive: blockStart, EndExclusive: blockStart.Add(blockSize)}, cachedResults) if err != nil { return false, fmt.Errorf("error querying cached block: %v", err) @@ -125,27 +125,20 @@ func TestPostingsListCacheDoesNotAffectBlockQueryResults(t *testing.T) { return false, errors.New("querying cached block was not exhaustive") } - if uncachedResults.Size() != cachedResults.Size() { + uncachedMap := uncachedResults.Map() + cachedMap := cachedResults.Map() + if uncachedMap.Len() != cachedMap.Len() { return false, fmt.Errorf( "uncached map size was: %d, but cached map sized was: %d", - uncachedResults.Size(), cachedResults.Size()) + uncachedMap.Len(), cachedMap.Len()) } - var checkMapsErr error - uncachedResults.WithMap(func(uncachedMap *ResultsMap) { - cachedResults.WithMap(func(cachedMap *ResultsMap) { - for _, entry := range uncachedMap.Iter() { - key := entry.Key() - _, ok := cachedMap.Get(key) - if !ok { - checkMapsErr = fmt.Errorf("cached map did not contain: %v", key) - return - } - } - }) - }) - if checkMapsErr != nil { - return false, checkMapsErr + for _, entry := range uncachedMap.Iter() { + key := entry.Key() + _, ok := cachedMap.Get(key) + if !ok { + return false, fmt.Errorf("cached map did not contain: %v", key) + } } } diff --git a/src/dbnode/storage/index/block_test.go b/src/dbnode/storage/index/block_test.go index a9b965e328..65fcb56db4 100644 --- a/src/dbnode/storage/index/block_test.go +++ b/src/dbnode/storage/index/block_test.go @@ -35,6 +35,7 @@ import ( "github.com/m3db/m3/src/m3ninx/index/segment" "github.com/m3db/m3/src/m3ninx/index/segment/mem" "github.com/m3db/m3/src/m3ninx/search" + "github.com/m3db/m3/src/x/resource" "github.com/m3db/m3x/ident" xlog "github.com/m3db/m3x/log" xtime "github.com/m3db/m3x/time" @@ -343,7 +344,8 @@ func TestBlockQueryAfterClose(t *testing.T) { require.Equal(t, start.Add(time.Hour), b.EndTime()) require.NoError(t, b.Close()) - _, err = b.Query(Query{}, QueryOptions{}, nil) + _, err = b.Query(resource.NewCancellableLifetime(), + Query{}, QueryOptions{}, nil) require.Error(t, err) } @@ -362,7 +364,8 @@ func TestBlockQueryExecutorError(t *testing.T) { return nil, fmt.Errorf("random-err") } - _, err = b.Query(Query{}, QueryOptions{}, nil) + _, err = b.Query(resource.NewCancellableLifetime(), + Query{}, QueryOptions{}, nil) require.Error(t, err) } @@ -383,7 +386,8 @@ func TestBlockQuerySegmentReaderError(t *testing.T) { randErr := fmt.Errorf("random-err") seg.EXPECT().Reader().Return(nil, randErr) - _, err = b.Query(Query{}, QueryOptions{}, nil) + _, err = b.Query(resource.NewCancellableLifetime(), + Query{}, QueryOptions{}, nil) require.Equal(t, randErr, err) } @@ -418,7 +422,8 @@ func TestBlockQueryAddResultsSegmentsError(t *testing.T) { randErr := fmt.Errorf("random-err") seg3.EXPECT().Reader().Return(nil, randErr) - _, err = b.Query(Query{}, QueryOptions{}, nil) + _, err = b.Query(resource.NewCancellableLifetime(), + Query{}, QueryOptions{}, nil) require.Equal(t, randErr, err) } @@ -443,7 +448,8 @@ func TestBlockMockQueryExecutorExecError(t *testing.T) { exec.EXPECT().Execute(gomock.Any()).Return(nil, fmt.Errorf("randomerr")), exec.EXPECT().Close(), ) - _, err = b.Query(Query{}, QueryOptions{}, nil) + _, err = b.Query(resource.NewCancellableLifetime(), + Query{}, QueryOptions{}, nil) require.Error(t, err) } @@ -474,7 +480,8 @@ func TestBlockMockQueryExecutorExecIterErr(t *testing.T) { dIter.EXPECT().Close(), exec.EXPECT().Close(), ) - _, err = b.Query(Query{}, QueryOptions{}, NewResults(testOpts)) + _, err = b.Query(resource.NewCancellableLifetime(), + Query{}, QueryOptions{}, NewResults(ResultsOptions{}, testOpts)) require.Error(t, err) } @@ -505,19 +512,20 @@ func TestBlockMockQueryExecutorExecLimit(t *testing.T) { dIter.EXPECT().Close().Return(nil), exec.EXPECT().Close().Return(nil), ) - results := NewResults(testOpts) - exhaustive, err := b.Query(Query{}, QueryOptions{Limit: 1}, results) + limit := 1 + results := NewResults(ResultsOptions{SizeLimit: limit}, testOpts) + exhaustive, err := b.Query(resource.NewCancellableLifetime(), + Query{}, QueryOptions{Limit: limit}, results) require.NoError(t, err) require.False(t, exhaustive) - results.WithMap(func(rMap *ResultsMap) { - require.Equal(t, 1, rMap.Len()) - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) - }) + rMap := results.Map() + require.Equal(t, 1, rMap.Len()) + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) } func TestBlockMockQueryExecutorExecIterCloseErr(t *testing.T) { @@ -545,8 +553,9 @@ func TestBlockMockQueryExecutorExecIterCloseErr(t *testing.T) { dIter.EXPECT().Close().Return(fmt.Errorf("random-err")), exec.EXPECT().Close().Return(nil), ) - results := NewResults(testOpts) - _, err = b.Query(Query{}, QueryOptions{}, results) + results := NewResults(ResultsOptions{}, testOpts) + _, err = b.Query(resource.NewCancellableLifetime(), + Query{}, QueryOptions{}, results) require.Error(t, err) } @@ -575,8 +584,9 @@ func TestBlockMockQueryExecutorExecIterExecCloseErr(t *testing.T) { dIter.EXPECT().Close().Return(nil), exec.EXPECT().Close().Return(fmt.Errorf("randomerr")), ) - results := NewResults(testOpts) - _, err = b.Query(Query{}, QueryOptions{}, results) + results := NewResults(ResultsOptions{}, testOpts) + _, err = b.Query(resource.NewCancellableLifetime(), + Query{}, QueryOptions{}, results) require.Error(t, err) } @@ -607,19 +617,20 @@ func TestBlockMockQueryLimit(t *testing.T) { dIter.EXPECT().Close().Return(nil), exec.EXPECT().Close().Return(nil), ) - results := NewResults(testOpts) - exhaustive, err := b.Query(Query{}, QueryOptions{Limit: 1}, results) + limit := 1 + results := NewResults(ResultsOptions{SizeLimit: 1}, testOpts) + exhaustive, err := b.Query(resource.NewCancellableLifetime(), + Query{}, QueryOptions{Limit: limit}, results) require.NoError(t, err) require.False(t, exhaustive) - results.WithMap(func(rMap *ResultsMap) { - require.Equal(t, 1, rMap.Len()) - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) - }) + rMap := results.Map() + require.Equal(t, 1, rMap.Len()) + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) } func TestBlockMockQueryLimitExhaustive(t *testing.T) { @@ -649,19 +660,20 @@ func TestBlockMockQueryLimitExhaustive(t *testing.T) { dIter.EXPECT().Close().Return(nil), exec.EXPECT().Close().Return(nil), ) - results := NewResults(testOpts) - exhaustive, err := b.Query(Query{}, QueryOptions{Limit: 1}, results) + limit := 2 + results := NewResults(ResultsOptions{SizeLimit: limit}, testOpts) + exhaustive, err := b.Query(resource.NewCancellableLifetime(), + Query{}, QueryOptions{Limit: limit}, results) require.NoError(t, err) require.True(t, exhaustive) - results.WithMap(func(rMap *ResultsMap) { - require.Equal(t, 1, rMap.Len()) - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) - }) + rMap := results.Map() + require.Equal(t, 1, rMap.Len()) + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) } func TestBlockMockQueryMergeResultsMapLimit(t *testing.T) { @@ -682,8 +694,9 @@ func TestBlockMockQueryMergeResultsMapLimit(t *testing.T) { return exec, nil } - results := NewResults(testOpts) - _, _, err = results.AddDocument(testDoc1()) + limit := 1 + results := NewResults(ResultsOptions{SizeLimit: limit}, testOpts) + _, err = results.AddDocuments([]doc.Document{testDoc1()}) require.NoError(t, err) dIter := doc.NewMockIterator(ctrl) @@ -694,18 +707,18 @@ func TestBlockMockQueryMergeResultsMapLimit(t *testing.T) { dIter.EXPECT().Close().Return(nil), exec.EXPECT().Close().Return(nil), ) - exhaustive, err := b.Query(Query{}, QueryOptions{Limit: 1}, results) + exhaustive, err := b.Query(resource.NewCancellableLifetime(), + Query{}, QueryOptions{Limit: limit}, results) require.NoError(t, err) require.False(t, exhaustive) - results.WithMap(func(rMap *ResultsMap) { - require.Equal(t, 1, rMap.Len()) - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) - }) + rMap := results.Map() + require.Equal(t, 1, rMap.Len()) + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) } func TestBlockMockQueryMergeResultsDupeID(t *testing.T) { @@ -725,8 +738,8 @@ func TestBlockMockQueryMergeResultsDupeID(t *testing.T) { return exec, nil } - results := NewResults(testOpts) - _, _, err = results.AddDocument(testDoc1()) + results := NewResults(ResultsOptions{}, testOpts) + _, err = results.AddDocuments([]doc.Document{testDoc1()}) require.NoError(t, err) dIter := doc.NewMockIterator(ctrl) @@ -741,24 +754,24 @@ func TestBlockMockQueryMergeResultsDupeID(t *testing.T) { dIter.EXPECT().Close().Return(nil), exec.EXPECT().Close().Return(nil), ) - exhaustive, err := b.Query(Query{}, QueryOptions{}, results) + exhaustive, err := b.Query(resource.NewCancellableLifetime(), + Query{}, QueryOptions{}, results) require.NoError(t, err) require.True(t, exhaustive) - results.WithMap(func(rMap *ResultsMap) { - require.Equal(t, 2, rMap.Len()) - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) + rMap := results.Map() + require.Equal(t, 2, rMap.Len()) + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) - t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( - ident.NewTagsIterator(t2))) - }) + t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( + ident.NewTagsIterator(t2))) } func TestBlockAddResultsAddsSegment(t *testing.T) { @@ -1142,25 +1155,25 @@ func TestBlockE2EInsertQuery(t *testing.T) { q, err := idx.NewRegexpQuery([]byte("bar"), []byte("b.*")) require.NoError(t, err) - results := NewResults(testOpts) - exhaustive, err := b.Query(Query{q}, QueryOptions{}, results) + results := NewResults(ResultsOptions{}, testOpts) + exhaustive, err := b.Query(resource.NewCancellableLifetime(), + Query{q}, QueryOptions{}, results) require.NoError(t, err) require.True(t, exhaustive) require.Equal(t, 2, results.Size()) - results.WithMap(func(rMap *ResultsMap) { - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) + rMap := results.Map() + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) - t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( - ident.NewTagsIterator(t2))) - }) + t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( + ident.NewTagsIterator(t2))) } func TestBlockE2EInsertQueryLimit(t *testing.T) { @@ -1209,32 +1222,34 @@ func TestBlockE2EInsertQueryLimit(t *testing.T) { q, err := idx.NewRegexpQuery([]byte("bar"), []byte("b.*")) require.NoError(t, err) - results := NewResults(testOpts) - exhaustive, err := b.Query(Query{q}, QueryOptions{Limit: 1}, results) + + limit := 1 + results := NewResults(ResultsOptions{SizeLimit: limit}, testOpts) + exhaustive, err := b.Query(resource.NewCancellableLifetime(), + Query{q}, QueryOptions{Limit: limit}, results) require.NoError(t, err) require.False(t, exhaustive) require.Equal(t, 1, results.Size()) - results.WithMap(func(rMap *ResultsMap) { - numFound := 0 - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - if ok { - numFound++ - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) - } + rMap := results.Map() + numFound := 0 + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + if ok { + numFound++ + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) + } - t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) - if ok { - numFound++ - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( - ident.NewTagsIterator(t2))) - } + t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) + if ok { + numFound++ + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( + ident.NewTagsIterator(t2))) + } - require.Equal(t, 1, numFound) - }) + require.Equal(t, 1, numFound) } func TestBlockE2EInsertAddResultsQuery(t *testing.T) { @@ -1288,25 +1303,25 @@ func TestBlockE2EInsertAddResultsQuery(t *testing.T) { q, err := idx.NewRegexpQuery([]byte("bar"), []byte("b.*")) require.NoError(t, err) - results := NewResults(testOpts) - exhaustive, err := b.Query(Query{q}, QueryOptions{}, results) + results := NewResults(ResultsOptions{}, testOpts) + exhaustive, err := b.Query(resource.NewCancellableLifetime(), + Query{q}, QueryOptions{}, results) require.NoError(t, err) require.True(t, exhaustive) require.Equal(t, 2, results.Size()) - results.WithMap(func(rMap *ResultsMap) { - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) + rMap := results.Map() + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) - t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( - ident.NewTagsIterator(t2))) - }) + t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( + ident.NewTagsIterator(t2))) } func TestBlockE2EInsertAddResultsMergeQuery(t *testing.T) { @@ -1352,25 +1367,25 @@ func TestBlockE2EInsertAddResultsMergeQuery(t *testing.T) { q, err := idx.NewRegexpQuery([]byte("bar"), []byte("b.*")) require.NoError(t, err) - results := NewResults(testOpts) - exhaustive, err := b.Query(Query{q}, QueryOptions{}, results) + results := NewResults(ResultsOptions{}, testOpts) + exhaustive, err := b.Query(resource.NewCancellableLifetime(), + Query{q}, QueryOptions{}, results) require.NoError(t, err) require.True(t, exhaustive) require.Equal(t, 2, results.Size()) - results.WithMap(func(rMap *ResultsMap) { - t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz")).Matches( - ident.NewTagsIterator(t1))) + rMap := results.Map() + t1, ok := rMap.Get(ident.StringID(string(testDoc1().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz")).Matches( + ident.NewTagsIterator(t1))) - t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) - require.True(t, ok) - require.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( - ident.NewTagsIterator(t2))) - }) + t2, ok := rMap.Get(ident.StringID(string(testDoc2().ID))) + require.True(t, ok) + require.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("bar", "baz", "some", "more")).Matches( + ident.NewTagsIterator(t2))) } func TestBlockWriteBackgroundCompact(t *testing.T) { diff --git a/src/dbnode/storage/index/index_mock.go b/src/dbnode/storage/index/index_mock.go index c63c70eef5..295e4b60f9 100644 --- a/src/dbnode/storage/index/index_mock.go +++ b/src/dbnode/storage/index/index_mock.go @@ -30,6 +30,7 @@ import ( "github.com/m3db/m3/src/dbnode/storage/bootstrap/result" "github.com/m3db/m3/src/m3ninx/doc" + "github.com/m3db/m3/src/x/resource" "github.com/m3db/m3x/context" "github.com/m3db/m3x/ident" time0 "github.com/m3db/m3x/time" @@ -60,36 +61,19 @@ func (m *MockResults) EXPECT() *MockResultsMockRecorder { return m.recorder } -// AddDocument mocks base method -func (m *MockResults) AddDocument(arg0 doc.Document) (bool, int, error) { +// AddDocuments mocks base method +func (m *MockResults) AddDocuments(arg0 []doc.Document) (int, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AddDocument", arg0) - ret0, _ := ret[0].(bool) - ret1, _ := ret[1].(int) - ret2, _ := ret[2].(error) - return ret0, ret1, ret2 -} - -// AddDocument indicates an expected call of AddDocument -func (mr *MockResultsMockRecorder) AddDocument(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddDocument", reflect.TypeOf((*MockResults)(nil).AddDocument), arg0) -} - -// AddIDAndTags mocks base method -func (m *MockResults) AddIDAndTags(arg0 ident.ID, arg1 ident.Tags) (bool, int, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AddIDAndTags", arg0, arg1) - ret0, _ := ret[0].(bool) - ret1, _ := ret[1].(int) - ret2, _ := ret[2].(error) - return ret0, ret1, ret2 + ret := m.ctrl.Call(m, "AddDocuments", arg0) + ret0, _ := ret[0].(int) + ret1, _ := ret[1].(error) + return ret0, ret1 } -// AddIDAndTags indicates an expected call of AddIDAndTags -func (mr *MockResultsMockRecorder) AddIDAndTags(arg0, arg1 interface{}) *gomock.Call { +// AddDocuments indicates an expected call of AddDocuments +func (mr *MockResultsMockRecorder) AddDocuments(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddIDAndTags", reflect.TypeOf((*MockResults)(nil).AddIDAndTags), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddDocuments", reflect.TypeOf((*MockResults)(nil).AddDocuments), arg0) } // Finalize mocks base method @@ -145,15 +129,15 @@ func (mr *MockResultsMockRecorder) NoFinalize() *gomock.Call { } // Reset mocks base method -func (m *MockResults) Reset(arg0 ident.ID) { +func (m *MockResults) Reset(arg0 ident.ID, arg1 ResultsOptions) { m.ctrl.T.Helper() - m.ctrl.Call(m, "Reset", arg0) + m.ctrl.Call(m, "Reset", arg0, arg1) } // Reset indicates an expected call of Reset -func (mr *MockResultsMockRecorder) Reset(arg0 interface{}) *gomock.Call { +func (mr *MockResultsMockRecorder) Reset(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Reset", reflect.TypeOf((*MockResults)(nil).Reset), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Reset", reflect.TypeOf((*MockResults)(nil).Reset), arg0, arg1) } // Size mocks base method @@ -278,18 +262,18 @@ func (mr *MockBlockMockRecorder) NeedsMutableSegmentsEvicted() *gomock.Call { } // Query mocks base method -func (m *MockBlock) Query(arg0 Query, arg1 QueryOptions, arg2 Results) (bool, error) { +func (m *MockBlock) Query(arg0 *resource.CancellableLifetime, arg1 Query, arg2 QueryOptions, arg3 Results) (bool, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Query", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "Query", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(bool) ret1, _ := ret[1].(error) return ret0, ret1 } // Query indicates an expected call of Query -func (mr *MockBlockMockRecorder) Query(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockBlockMockRecorder) Query(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Query", reflect.TypeOf((*MockBlock)(nil).Query), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Query", reflect.TypeOf((*MockBlock)(nil).Query), arg0, arg1, arg2, arg3) } // Seal mocks base method diff --git a/src/dbnode/storage/index/options.go b/src/dbnode/storage/index/options.go index 7e6d72f7c2..0ff34ad007 100644 --- a/src/dbnode/storage/index/options.go +++ b/src/dbnode/storage/index/options.go @@ -140,7 +140,7 @@ func NewOptions() Options { foregroundCompactionPlannerOpts: defaultForegroundCompactionOpts, backgroundCompactionPlannerOpts: defaultBackgroundCompactionOpts, } - resultsPool.Init(func() Results { return NewResults(opts) }) + resultsPool.Init(func() Results { return NewResults(ResultsOptions{}, opts) }) return opts } diff --git a/src/dbnode/storage/index/results.go b/src/dbnode/storage/index/results.go index 150d2c3704..c0b32571ab 100644 --- a/src/dbnode/storage/index/results.go +++ b/src/dbnode/storage/index/results.go @@ -36,7 +36,10 @@ var ( type results struct { sync.RWMutex - nsID ident.ID + + nsID ident.ID + opts ResultsOptions + resultsMap *ResultsMap idPool ident.Pool @@ -47,126 +50,97 @@ type results struct { } // NewResults returns a new results object. -func NewResults(opts Options) Results { +func NewResults(opts ResultsOptions, indexOpts Options) Results { return &results{ - resultsMap: newResultsMap(opts.IdentifierPool()), - idPool: opts.IdentifierPool(), - bytesPool: opts.CheckedBytesPool(), - pool: opts.ResultsPool(), + opts: opts, + resultsMap: newResultsMap(indexOpts.IdentifierPool()), + idPool: indexOpts.IdentifierPool(), + bytesPool: indexOpts.CheckedBytesPool(), + pool: indexOpts.ResultsPool(), } } -func (r *results) AddDocument( - d doc.Document, -) (added bool, size int, err error) { +func (r *results) Reset(nsID ident.ID, opts ResultsOptions) { r.Lock() - added, size, err = r.addDocumentWithLock(d) - r.Unlock() - return -} -func (r *results) addDocumentWithLock( - d doc.Document, -) (added bool, size int, err error) { - added = false - if len(d.ID) == 0 { - return added, r.resultsMap.Len(), errUnableToAddResultMissingID - } + r.opts = opts - // NB: can cast the []byte -> ident.ID to avoid an alloc - // before we're sure we need it. - tsID := ident.BytesID(d.ID) + // finalize existing held nsID + if r.nsID != nil { + r.nsID.Finalize() + } + // make an independent copy of the new nsID + if nsID != nil { + nsID = r.idPool.Clone(nsID) + } + r.nsID = nsID - // check if it already exists in the map. - if r.resultsMap.Contains(tsID) { - return added, r.resultsMap.Len(), nil + // reset all values from map first + for _, entry := range r.resultsMap.Iter() { + tags := entry.Value() + tags.Finalize() } - // i.e. it doesn't exist in the map, so we create the tags wrapping - // fields prodided by the document. - tags := r.cloneTagsFromFields(d.Fields) + // reset all keys in the map next + r.resultsMap.Reset() - // We use Set() instead of SetUnsafe to ensure we're taking a copy of - // the tsID's bytes. - r.resultsMap.Set(tsID, tags) + // NB: could do keys+value in one step but I'm trying to avoid + // using an internal method of a code-gen'd type. - added = true - return added, r.resultsMap.Len(), nil + r.Unlock() } -func (r *results) AddDocumentsBatch( - batch []doc.Document, - opts AddDocumentsBatchResultsOptions, -) (numPartialUpdates int, size int, err error) { +func (r *results) AddDocuments(batch []doc.Document) (int, error) { r.Lock() - numPartialUpdates, size, err = r.addDocumentsBatchWithLock(batch, opts) + err := r.addDocumentsBatchWithLock(batch) + size := r.resultsMap.Len() r.Unlock() - return + return size, err } -func (r *results) addDocumentsBatchWithLock( - batch []doc.Document, - opts AddDocumentsBatchResultsOptions, -) (numPartialUpdates int, size int, err error) { - numPartialUpdates = 0 - size = r.resultsMap.Len() +func (r *results) addDocumentsBatchWithLock(batch []doc.Document) error { for i := range batch { - var added bool - added, size, err = r.addDocumentWithLock(batch[i]) + _, size, err := r.addDocumentWithLock(batch[i]) if err != nil { - return numPartialUpdates, size, err - } - if !added { - if !opts.AllowPartialUpdates { - return numPartialUpdates, size, errResultAlreadyExistsNoPartialUpdates - } - numPartialUpdates++ + return err } - if opts.LimitSize > 0 && size >= opts.LimitSize { + if r.opts.SizeLimit > 0 && size >= r.opts.SizeLimit { // Early return if limit enforced and we hit our limit break } } - return numPartialUpdates, size, nil -} - -func (r *results) AddIDAndTags( - id ident.ID, - tags ident.Tags, -) (added bool, size int, err error) { - r.Lock() - added, size, err = r.addIDAndTagsWithLock(id, tags) - r.Unlock() - return + return nil } -func (r *results) addIDAndTagsWithLock( - id ident.ID, - tags ident.Tags, -) (added bool, size int, err error) { - added = false - bytesID := ident.BytesID(id.Bytes()) - if len(bytesID) == 0 { +func (r *results) addDocumentWithLock( + d doc.Document, +) (bool, int, error) { + added := false + if len(d.ID) == 0 { return added, r.resultsMap.Len(), errUnableToAddResultMissingID } + // NB: can cast the []byte -> ident.ID to avoid an alloc + // before we're sure we need it. + tsID := ident.BytesID(d.ID) + // check if it already exists in the map. - if r.resultsMap.Contains(bytesID) { + if r.resultsMap.Contains(tsID) { return added, r.resultsMap.Len(), nil } + // i.e. it doesn't exist in the map, so we create the tags wrapping + // fields prodided by the document. + tags := r.cloneTagsFromFields(d.Fields) + // We use Set() instead of SetUnsafe to ensure we're taking a copy of // the tsID's bytes. - r.resultsMap.Set(bytesID, r.cloneTags(tags)) + r.resultsMap.Set(tsID, tags) added = true return added, r.resultsMap.Len(), nil } -func (r *results) cloneTags(tags ident.Tags) ident.Tags { - return r.idPool.CloneTags(tags) -} - func (r *results) cloneTagsFromFields(fields doc.Fields) ident.Tags { tags := r.idPool.Tags() for _, f := range fields { @@ -185,10 +159,11 @@ func (r *results) Namespace() ident.ID { return v } -func (r *results) WithMap(fn func(results *ResultsMap)) { +func (r *results) Map() *ResultsMap { r.RLock() - fn(r.resultsMap) + v := r.resultsMap r.RUnlock() + return v } func (r *results) Size() int { @@ -198,33 +173,6 @@ func (r *results) Size() int { return v } -func (r *results) Reset(nsID ident.ID) { - r.Lock() - defer r.Unlock() - - // finalize existing held nsID - if r.nsID != nil { - r.nsID.Finalize() - } - // make an independent copy of the new nsID - if nsID != nil { - nsID = r.idPool.Clone(nsID) - } - r.nsID = nsID - - // reset all values from map first - for _, entry := range r.resultsMap.Iter() { - tags := entry.Value() - tags.Finalize() - } - - // reset all keys in the map next - r.resultsMap.Reset() - - // NB: could do keys+value in one step but I'm trying to avoid - // using an internal method of a code-gen'd type. -} - func (r *results) Finalize() { r.RLock() noFinalize := r.noFinalize @@ -234,7 +182,7 @@ func (r *results) Finalize() { return } - r.Reset(nil) + r.Reset(nil, ResultsOptions{}) if r.pool == nil { return @@ -244,7 +192,6 @@ func (r *results) Finalize() { func (r *results) NoFinalize() { r.Lock() - defer r.Unlock() // Ensure neither the results object itself, or any of its underlying // IDs and tags will be finalized. @@ -254,4 +201,6 @@ func (r *results) NoFinalize() { id.NoFinalize() tags.NoFinalize() } + + r.Unlock() } diff --git a/src/dbnode/storage/index/results_test.go b/src/dbnode/storage/index/results_test.go index 000582b7aa..80ec90d6a0 100644 --- a/src/dbnode/storage/index/results_test.go +++ b/src/dbnode/storage/index/results_test.go @@ -27,6 +27,7 @@ import ( "github.com/m3db/m3/src/m3ninx/doc" xtest "github.com/m3db/m3/src/x/test" "github.com/m3db/m3x/ident" + "github.com/m3db/m3x/pool" "github.com/stretchr/testify/require" ) @@ -37,203 +38,138 @@ var ( ) func init() { - testOpts = NewOptions() + // Use size of 1 to test edge cases + docArrayPool := doc.NewDocumentArrayPool(doc.DocumentArrayPoolOpts{ + Options: pool.NewObjectPoolOptions().SetSize(1), + Capacity: 1, + MaxCapacity: 1, + }) + docArrayPool.Init() + + testOpts = NewOptions().SetDocumentArrayPool(docArrayPool) } func TestResultsInsertInvalid(t *testing.T) { - res := NewResults(testOpts) + res := NewResults(ResultsOptions{}, testOpts) dInvalid := doc.Document{ID: nil} - added, size, err := res.AddDocument(dInvalid) + size, err := res.AddDocuments([]doc.Document{dInvalid}) require.Error(t, err) - require.False(t, added) require.Equal(t, 0, size) } func TestResultsInsertIdempotency(t *testing.T) { - res := NewResults(testOpts) + res := NewResults(ResultsOptions{}, testOpts) dValid := doc.Document{ID: []byte("abc")} - added, size, err := res.AddDocument(dValid) + size, err := res.AddDocuments([]doc.Document{dValid}) require.NoError(t, err) - require.True(t, added) require.Equal(t, 1, size) - added, size, err = res.AddDocument(dValid) + size, err = res.AddDocuments([]doc.Document{dValid}) require.NoError(t, err) - require.False(t, added) require.Equal(t, 1, size) } func TestResultsFirstInsertWins(t *testing.T) { - res := NewResults(testOpts) + res := NewResults(ResultsOptions{}, testOpts) d1 := doc.Document{ID: []byte("abc")} - added, size, err := res.AddDocument(d1) + size, err := res.AddDocuments([]doc.Document{d1}) require.NoError(t, err) - require.True(t, added) require.Equal(t, 1, size) - res.WithMap(func(rMap *ResultsMap) { - tags, ok := rMap.Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) - }) + tags, ok := res.Map().Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) d2 := doc.Document{ID: []byte("abc"), Fields: doc.Fields{ doc.Field{Name: []byte("foo"), Value: []byte("bar")}, }} - added, size, err = res.AddDocument(d2) + size, err = res.AddDocuments([]doc.Document{d2}) require.NoError(t, err) - require.False(t, added) require.Equal(t, 1, size) - res.WithMap(func(rMap *ResultsMap) { - tags, ok := rMap.Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) - }) + tags, ok = res.Map().Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) } func TestResultsInsertContains(t *testing.T) { - res := NewResults(testOpts) + res := NewResults(ResultsOptions{}, testOpts) dValid := doc.Document{ID: []byte("abc")} - added, size, err := res.AddDocument(dValid) + size, err := res.AddDocuments([]doc.Document{dValid}) require.NoError(t, err) - require.True(t, added) require.Equal(t, 1, size) - res.WithMap(func(rMap *ResultsMap) { - tags, ok := rMap.Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) - }) + tags, ok := res.Map().Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) } func TestResultsInsertCopies(t *testing.T) { - res := NewResults(testOpts) + res := NewResults(ResultsOptions{}, testOpts) dValid := doc.Document{ID: []byte("abc"), Fields: []doc.Field{ doc.Field{Name: []byte("name"), Value: []byte("value")}, }} - added, size, err := res.AddDocument(dValid) + size, err := res.AddDocuments([]doc.Document{dValid}) require.NoError(t, err) - require.True(t, added) require.Equal(t, 1, size) found := false // our genny generated maps don't provide access to MapEntry directly, // so we iterate over the map to find the added entry. Could avoid this // in the future if we expose `func (m *Map) Entry(k Key) Entry {}` - res.WithMap(func(rMap *ResultsMap) { - for _, entry := range rMap.Iter() { - // see if this key has the same value as the added document's ID. - key := entry.Key().Bytes() - if !bytes.Equal(dValid.ID, key) { - continue - } - found = true - // ensure the underlying []byte for ID/Fields is at a different address - // than the original. - require.False(t, xtest.ByteSlicesBackedBySameData(key, dValid.ID)) - tags := entry.Value().Values() - for _, f := range dValid.Fields { - fName := f.Name - fValue := f.Value - for _, tag := range tags { - tName := tag.Name.Bytes() - tValue := tag.Value.Bytes() - if !bytes.Equal(fName, tName) || !bytes.Equal(fValue, tValue) { - continue - } - require.False(t, xtest.ByteSlicesBackedBySameData(fName, tName)) - require.False(t, xtest.ByteSlicesBackedBySameData(fValue, tValue)) + for _, entry := range res.Map().Iter() { + // see if this key has the same value as the added document's ID. + key := entry.Key().Bytes() + if !bytes.Equal(dValid.ID, key) { + continue + } + found = true + // ensure the underlying []byte for ID/Fields is at a different address + // than the original. + require.False(t, xtest.ByteSlicesBackedBySameData(key, dValid.ID)) + tags := entry.Value().Values() + for _, f := range dValid.Fields { + fName := f.Name + fValue := f.Value + for _, tag := range tags { + tName := tag.Name.Bytes() + tValue := tag.Value.Bytes() + if !bytes.Equal(fName, tName) || !bytes.Equal(fValue, tValue) { + continue } + require.False(t, xtest.ByteSlicesBackedBySameData(fName, tName)) + require.False(t, xtest.ByteSlicesBackedBySameData(fValue, tValue)) } } + } - require.True(t, found) - }) -} - -func TestResultsInsertIDAndTagsInvalid(t *testing.T) { - res := NewResults(testOpts) - added, size, err := res.AddIDAndTags(ident.BytesID(nil), ident.Tags{}) - require.Error(t, err) - require.False(t, added) - require.Equal(t, 0, size) -} - -func TestResultsInsertIDAndTagsIdempotency(t *testing.T) { - res := NewResults(testOpts) - dValid := doc.Document{ID: []byte("abc")} - added, size, err := res.AddIDAndTags(ident.BytesID(dValid.ID), tagsFromFields(dValid.Fields)) - require.NoError(t, err) - require.True(t, added) - require.Equal(t, 1, size) - - added, size, err = res.AddIDAndTags(ident.BytesID(dValid.ID), tagsFromFields(dValid.Fields)) - require.NoError(t, err) - require.False(t, added) - require.Equal(t, 1, size) -} - -func TestResultsIDAndTagsFirstInsertWins(t *testing.T) { - res := NewResults(testOpts) - d1 := doc.Document{ID: []byte("abc")} - added, size, err := res.AddIDAndTags(ident.BytesID(d1.ID), tagsFromFields(d1.Fields)) - require.NoError(t, err) - require.True(t, added) - require.Equal(t, 1, size) - - res.WithMap(func(rMap *ResultsMap) { - tags, ok := rMap.Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) - }) - - d2 := doc.Document{ID: []byte("abc"), - Fields: doc.Fields{ - doc.Field{Name: []byte("foo"), Value: []byte("bar")}, - }} - added, size, err = res.AddIDAndTags(ident.BytesID(d2.ID), tagsFromFields(d2.Fields)) - require.NoError(t, err) - require.False(t, added) - require.Equal(t, 1, size) - - res.WithMap(func(rMap *ResultsMap) { - tags, ok := rMap.Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) - }) + require.True(t, found) } func TestResultsReset(t *testing.T) { - res := NewResults(testOpts) + res := NewResults(ResultsOptions{}, testOpts) d1 := doc.Document{ID: []byte("abc")} - added, size, err := res.AddDocument(d1) + size, err := res.AddDocuments([]doc.Document{d1}) require.NoError(t, err) - require.True(t, added) require.Equal(t, 1, size) - res.WithMap(func(rMap *ResultsMap) { - tags, ok := rMap.Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) - }) - - res.Reset(nil) + tags, ok := res.Map().Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) - res.WithMap(func(rMap *ResultsMap) { - tags, ok := rMap.Get(ident.StringID("abc")) - require.False(t, ok) - require.Equal(t, 0, len(tags.Values())) - require.Equal(t, 0, res.Size()) - }) + res.Reset(nil, ResultsOptions{}) + _, ok = res.Map().Get(ident.StringID("abc")) + require.False(t, ok) + require.Equal(t, 0, len(tags.Values())) + require.Equal(t, 0, res.Size()) } func TestResultsResetNamespaceClones(t *testing.T) { - res := NewResults(testOpts) + res := NewResults(ResultsOptions{}, testOpts) require.Equal(t, nil, res.Namespace()) nsID := ident.StringID("something") - res.Reset(nsID) + res.Reset(nsID, ResultsOptions{}) nsID.Finalize() require.Equal(t, "something", res.Namespace().String()) } @@ -251,54 +187,46 @@ func tagsFromFields(fields []doc.Field) ident.Tags { func TestFinalize(t *testing.T) { // Create a Results and insert some data. - res := NewResults(testOpts) + res := NewResults(ResultsOptions{}, testOpts) d1 := doc.Document{ID: []byte("abc")} - added, size, err := res.AddDocument(d1) + size, err := res.AddDocuments([]doc.Document{d1}) require.NoError(t, err) - require.True(t, added) require.Equal(t, 1, size) // Ensure the data is present. - res.WithMap(func(rMap *ResultsMap) { - tags, ok := rMap.Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) - }) + tags, ok := res.Map().Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) // Call Finalize() to reset the Results. res.Finalize() // Ensure data was removed by call to Finalize(). - res.WithMap(func(rMap *ResultsMap) { - tags, ok := rMap.Get(ident.StringID("abc")) - require.False(t, ok) - require.Equal(t, 0, len(tags.Values())) - require.Equal(t, 0, res.Size()) - - for _, entry := range rMap.Iter() { - id, _ := entry.Key(), entry.Value() - require.False(t, id.IsNoFinalize()) - // TODO(rartoul): Could verify tags are NoFinalize() as well if - // they had that method. - } - }) + tags, ok = res.Map().Get(ident.StringID("abc")) + require.False(t, ok) + require.Equal(t, 0, len(tags.Values())) + require.Equal(t, 0, res.Size()) + + for _, entry := range res.Map().Iter() { + id, _ := entry.Key(), entry.Value() + require.False(t, id.IsNoFinalize()) + // TODO(rartoul): Could verify tags are NoFinalize() as well if + // they had that method. + } } func TestNoFinalize(t *testing.T) { // Create a Results and insert some data. - res := NewResults(testOpts) + res := NewResults(ResultsOptions{}, testOpts) d1 := doc.Document{ID: []byte("abc")} - added, size, err := res.AddDocument(d1) + size, err := res.AddDocuments([]doc.Document{d1}) require.NoError(t, err) - require.True(t, added) require.Equal(t, 1, size) // Ensure the data is present. - res.WithMap(func(rMap *ResultsMap) { - tags, ok := rMap.Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) - }) + tags, ok := res.Map().Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) // Call to NoFinalize indicates that subsequent call // to finalize should be a no-op. @@ -306,17 +234,15 @@ func TestNoFinalize(t *testing.T) { res.Finalize() // Ensure data was not removed by call to Finalize(). - res.WithMap(func(rMap *ResultsMap) { - tags, ok := rMap.Get(ident.StringID("abc")) - require.True(t, ok) - require.Equal(t, 0, len(tags.Values())) - require.Equal(t, 1, res.Size()) - - for _, entry := range rMap.Iter() { - id, _ := entry.Key(), entry.Value() - require.True(t, id.IsNoFinalize()) - // TODO(rartoul): Could verify tags are NoFinalize() as well if - // they had that method. - } - }) + tags, ok = res.Map().Get(ident.StringID("abc")) + require.True(t, ok) + require.Equal(t, 0, len(tags.Values())) + require.Equal(t, 1, res.Size()) + + for _, entry := range res.Map().Iter() { + id, _ := entry.Key(), entry.Value() + require.True(t, id.IsNoFinalize()) + // TODO(rartoul): Could verify tags are NoFinalize() as well if + // they had that method. + } } diff --git a/src/dbnode/storage/index/types.go b/src/dbnode/storage/index/types.go index 87bb14e091..ba107ccaba 100644 --- a/src/dbnode/storage/index/types.go +++ b/src/dbnode/storage/index/types.go @@ -33,6 +33,7 @@ import ( "github.com/m3db/m3/src/m3ninx/index/segment/builder" "github.com/m3db/m3/src/m3ninx/index/segment/fst" "github.com/m3db/m3/src/m3ninx/index/segment/mem" + "github.com/m3db/m3/src/x/resource" "github.com/m3db/m3x/context" "github.com/m3db/m3x/ident" "github.com/m3db/m3x/instrument" @@ -81,71 +82,44 @@ type QueryResults struct { // Results is a collection of results for a query, it is synchronized // when access to the results set is used as documented by the methods. +// It cannot be written to after it is sealed, until it's reopened by +// resetting the results. type Results interface { + // Reset resets the Results object to initial state. + Reset(nsID ident.ID, opts ResultsOptions) + // Namespace returns the namespace associated with the result. Namespace() ident.ID - // WithMap executes a function that has access to a results map - // from seriesID -> seriesTags, comprising index results. - // A function is required to ensure that the results is used - // while a read lock for the results is held. - WithMap(fn func(results *ResultsMap)) + // Size returns the number of IDs tracked. + Size() int - // Reset resets the Results object to initial state. - Reset(nsID ident.ID) + // Map returns the results map from seriesID -> seriesTags, comprising + // index results. + Map() *ResultsMap + + // AddDocuments adds the batch of documents to the results set, it will + // take a copy of the bytes backing the documents so the original can be + // modified after this function returns without affecting the results map. + // If documents with duplicate IDs are added, they are simply ignored and + // the first document added with an ID is returned. + AddDocuments(batch []doc.Document) (size int, err error) // Finalize releases any resources held by the Results object, // including returning it to a backing pool. Finalize() - // NoFinalize marks the Results such that a subsequent call to Finalize() will - // be a no-op and will not return the object to the pool or release any of its - // resources. + // NoFinalize marks the Results such that a subsequent call to Finalize() + // will be a no-op and will not return the object to the pool or release any + // of its resources. NoFinalize() +} - // Size returns the number of IDs tracked. - Size() int - - // Add converts the provided document to a metric and adds it to the results. - // This method makes a copy of the bytes backing the document, so the original - // may be modified after this function returns without affecting the results map. - // NB: it returns a bool to indicate if the doc was added (it won't be added - // if it already existed in the ResultsMap). - AddDocument( - document doc.Document, - ) (added bool, size int, err error) - - // AddDocumentsBatch adds the batch of documents to the results set, it will - // take a copy of the bytes backing the documents so the original can be modified - // after this function returns without affecting the results map. - AddDocumentsBatch( - batch []doc.Document, - opts AddDocumentsBatchResultsOptions, - ) (numPartialUpdates int, size int, err error) - - // AddIDAndTagsOrFinalize adds ID and tags to the results and takes ownership - // if it does not exist, otherwise if it's already contained it returns added - // false. - AddIDAndTags( - id ident.ID, - tags ident.Tags, - ) (added bool, size int, err error) -} - -// AddDocumentsBatchResultsOptions is a set of options to use when adding -// results for a query. -type AddDocumentsBatchResultsOptions struct { - // If AllowPartialUpdates is true the query result will continue to add results - // even if it encounters an error attempting to add document that already exists - // in the results set. - // If false, on the other hand, then any errors encountered adding a document - // that already exists to the results set will cause the addition of a batch to - // fail and none of the documents in the batch will be added to the results. - AllowPartialUpdates bool - - // LimitSize will limit the total results set to a given limit and if overflown - // will return early successfully. - LimitSize int +// ResultsOptions is a set of options to use for results. +type ResultsOptions struct { + // SizeLimit will limit the total results set to a given limit and if + // overflown will return early successfully. + SizeLimit int } // ResultsAllocator allocates Results types. @@ -192,6 +166,7 @@ type Block interface { // Query resolves the given query into known IDs. Query( + cancellable *resource.CancellableLifetime, query Query, opts QueryOptions, results Results, diff --git a/src/dbnode/storage/index_block_test.go b/src/dbnode/storage/index_block_test.go index cd8a25bb4f..c303727c51 100644 --- a/src/dbnode/storage/index_block_test.go +++ b/src/dbnode/storage/index_block_test.go @@ -526,7 +526,7 @@ func TestNamespaceIndexBlockQuery(t *testing.T) { StartInclusive: t0, EndExclusive: now.Add(time.Minute), } - b0.EXPECT().Query(q, qOpts, gomock.Any()).Return(true, nil) + b0.EXPECT().Query(gomock.Any(), q, qOpts, gomock.Any()).Return(true, nil) _, err = idx.Query(ctx, q, qOpts) require.NoError(t, err) @@ -535,8 +535,8 @@ func TestNamespaceIndexBlockQuery(t *testing.T) { StartInclusive: t0, EndExclusive: t2.Add(time.Minute), } - b0.EXPECT().Query(q, qOpts, gomock.Any()).Return(true, nil) - b1.EXPECT().Query(q, qOpts, gomock.Any()).Return(true, nil) + b0.EXPECT().Query(gomock.Any(), q, qOpts, gomock.Any()).Return(true, nil) + b1.EXPECT().Query(gomock.Any(), q, qOpts, gomock.Any()).Return(true, nil) _, err = idx.Query(ctx, q, qOpts) require.NoError(t, err) @@ -545,7 +545,7 @@ func TestNamespaceIndexBlockQuery(t *testing.T) { StartInclusive: t0, EndExclusive: t0.Add(time.Minute), } - b0.EXPECT().Query(q, qOpts, gomock.Any()).Return(false, nil) + b0.EXPECT().Query(gomock.Any(), q, qOpts, gomock.Any()).Return(false, nil) _, err = idx.Query(ctx, q, qOpts) require.NoError(t, err) } diff --git a/src/dbnode/storage/index_query_concurrent_test.go b/src/dbnode/storage/index_query_concurrent_test.go index 97b52cc4e8..1b1405d164 100644 --- a/src/dbnode/storage/index_query_concurrent_test.go +++ b/src/dbnode/storage/index_query_concurrent_test.go @@ -33,6 +33,7 @@ import ( "github.com/m3db/m3/src/dbnode/storage/index/convert" "github.com/m3db/m3/src/m3ninx/doc" "github.com/m3db/m3/src/m3ninx/idx" + "github.com/m3db/m3/src/x/resource" "github.com/m3db/m3x/context" xsync "github.com/m3db/m3x/sync" xtest "github.com/m3db/m3x/test" @@ -222,17 +223,27 @@ func testNamespaceIndexHighConcurrentQueries( if opts.blockErrors { mockBlock.EXPECT(). - Query(gomock.Any(), gomock.Any(), gomock.Any()). - DoAndReturn(func(q index.Query, opts index.QueryOptions, r index.Results) (bool, error) { + Query(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func( + l *resource.CancellableLifetime, + q index.Query, + opts index.QueryOptions, + r index.Results, + ) (bool, error) { return false, errors.New("some-error") }). AnyTimes() } else { mockBlock.EXPECT(). - Query(gomock.Any(), gomock.Any(), gomock.Any()). - DoAndReturn(func(q index.Query, opts index.QueryOptions, r index.Results) (bool, error) { + Query(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func( + c *resource.CancellableLifetime, + q index.Query, + opts index.QueryOptions, + r index.Results, + ) (bool, error) { timeoutWg.Wait() - return block.Query(q, opts, r) + return block.Query(c, q, opts, r) }). AnyTimes() } @@ -313,29 +324,26 @@ func testNamespaceIndexHighConcurrentQueries( // Read the results concurrently too hits := make(map[string]struct{}, results.Results.Size()) - results.Results.WithMap(func(rMap *index.ResultsMap) { - for _, entry := range rMap.Iter() { - id := entry.Key().String() - - doc, err := convert.FromMetricNoClone(entry.Key(), entry.Value()) - require.NoError(t, err) - if err != nil { - continue // this will fail the test anyway, but don't want to panic - } - - expectedDoc, ok := expectedResults[id] - require.True(t, ok) - if !ok { - continue // this will fail the test anyway, but don't want to panic - } - - require.Equal(t, expectedDoc, doc) - hits[id] = struct{}{} + for _, entry := range results.Results.Map().Iter() { + id := entry.Key().String() + + doc, err := convert.FromMetricNoClone(entry.Key(), entry.Value()) + require.NoError(t, err) + if err != nil { + continue // this will fail the test anyway, but don't want to panic } - expectedHits := idsPerBlock * (k + 1) - require.Equal(t, expectedHits, len(hits)) - }) + expectedDoc, ok := expectedResults[id] + require.True(t, ok) + if !ok { + continue // this will fail the test anyway, but don't want to panic + } + + require.Equal(t, expectedDoc, doc) + hits[id] = struct{}{} + } + expectedHits := idsPerBlock * (k + 1) + require.Equal(t, expectedHits, len(hits)) } }() } diff --git a/src/dbnode/storage/index_queue_test.go b/src/dbnode/storage/index_queue_test.go index 9c05b15ac9..7001086e8b 100644 --- a/src/dbnode/storage/index_queue_test.go +++ b/src/dbnode/storage/index_queue_test.go @@ -316,11 +316,9 @@ func TestNamespaceIndexInsertQuery(t *testing.T) { assert.True(t, res.Exhaustive) results := res.Results assert.Equal(t, "testns1", results.Namespace().String()) - results.WithMap(func(rMap *index.ResultsMap) { - tags, ok := rMap.Get(ident.StringID("foo")) - assert.True(t, ok) - assert.True(t, ident.NewTagIterMatcher( - ident.MustNewTagStringsIterator("name", "value")).Matches( - ident.NewTagsIterator(tags))) - }) + tags, ok := results.Map().Get(ident.StringID("foo")) + assert.True(t, ok) + assert.True(t, ident.NewTagIterMatcher( + ident.MustNewTagStringsIterator("name", "value")).Matches( + ident.NewTagsIterator(tags))) } diff --git a/src/x/resource/lifetime.go b/src/x/resource/lifetime.go new file mode 100644 index 0000000000..92319c673a --- /dev/null +++ b/src/x/resource/lifetime.go @@ -0,0 +1,67 @@ +// Copyright (c) 2019 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package resource + +import "sync" + +// CancellableLifetime describes a lifetime for a resource that +// allows checking out the resource and returning it and once +// cancelled will not allow any further checkouts. +type CancellableLifetime struct { + mu sync.RWMutex + checkouts int + cancelled bool +} + +// NewCancellableLifetime returns a new cancellable resource lifetime. +func NewCancellableLifetime() *CancellableLifetime { + return &CancellableLifetime{} +} + +// TryCheckout will try to checkout the resource, if the lifetime +// is already cancelled this will return false, otherwise it will return +// true and guarantee the lifetime is not cancelled until the checkout +// is returned. +func (l *CancellableLifetime) TryCheckout() bool { + l.mu.RLock() + if l.cancelled { + // Already cancelled, close the RLock don't need to keep it open + l.mu.RUnlock() + return false + } + + // Keep the RLock open + return true +} + +// ReleaseCheckout will decrement the number of current checkouts. +func (l *CancellableLifetime) ReleaseCheckout() { + l.mu.RUnlock() +} + +// Cancel will wait for all current checkouts to be returned +// and then will cancel the lifetime so that it cannot be +// checked out any longer. +func (l *CancellableLifetime) Cancel() { + l.mu.Lock() + l.cancelled = true + l.mu.Unlock() +} diff --git a/src/x/resource/lifetime_test.go b/src/x/resource/lifetime_test.go new file mode 100644 index 0000000000..f6f23b122d --- /dev/null +++ b/src/x/resource/lifetime_test.go @@ -0,0 +1,69 @@ +// Copyright (c) 2019 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package resource + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + "github.com/uber-go/atomic" +) + +func TestCancellableLifetime(t *testing.T) { + l := NewCancellableLifetime() + + checkouts := 42 + + for i := 0; i < checkouts; i++ { + ok := l.TryCheckout() + require.True(t, ok) + } + + // Launch cancel goroutine + cancelDone := atomic.NewBool(false) + go func() { + l.Cancel() + cancelDone.Store(true) + }() + + for i := 0; i < checkouts; i++ { + time.Sleep(2 * time.Millisecond) + + // Ensure not done yet until final release + done := cancelDone.Load() + require.False(t, done) + + l.ReleaseCheckout() + } + + // Ensure cannot checkout any longer + ok := l.TryCheckout() + require.False(t, ok) + + // Ensure that cancel finished + for { + if cancelDone.Load() { + break + } + time.Sleep(2 * time.Millisecond) + } +} From c94cb187e2dc764e00055eac5862ebd69963fd04 Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Wed, 20 Mar 2019 23:25:07 -0400 Subject: [PATCH 03/12] Fix typos in comments --- src/dbnode/storage/index/block.go | 2 +- src/dbnode/storage/index/results.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/dbnode/storage/index/block.go b/src/dbnode/storage/index/block.go index 2408b63657..43623f2bfe 100644 --- a/src/dbnode/storage/index/block.go +++ b/src/dbnode/storage/index/block.go @@ -807,7 +807,7 @@ func (b *block) Query( } } - // Put last batch if remainding + // Add last batch to results if remaining. if len(batch) > 0 { batch, size, err = b.addQueryResults(cancellable, results, batch) if err != nil { diff --git a/src/dbnode/storage/index/results.go b/src/dbnode/storage/index/results.go index c0b32571ab..774c2ac5fc 100644 --- a/src/dbnode/storage/index/results.go +++ b/src/dbnode/storage/index/results.go @@ -182,6 +182,7 @@ func (r *results) Finalize() { return } + // Reset locks so cannot hold onto lock for call to Finalize. r.Reset(nil, ResultsOptions{}) if r.pool == nil { @@ -193,7 +194,7 @@ func (r *results) Finalize() { func (r *results) NoFinalize() { r.Lock() - // Ensure neither the results object itself, or any of its underlying + // Ensure neither the results object itself, nor any of its underlying // IDs and tags will be finalized. r.noFinalize = true for _, entry := range r.resultsMap.Iter() { From 6f434a26eba9f68ef5a8031c0f01d0afedc76344 Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Wed, 20 Mar 2019 23:28:48 -0400 Subject: [PATCH 04/12] Fix build error --- src/dbnode/server/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dbnode/server/server.go b/src/dbnode/server/server.go index d1e87f888d..c04df075c5 100644 --- a/src/dbnode/server/server.go +++ b/src/dbnode/server/server.go @@ -1223,7 +1223,7 @@ func withEncodingAndPoolingOptions( SetCheckedBytesPool(bytesPool). SetResultsPool(resultsPool) - resultsPool.Init(func() index.Results { return index.NewResults(indexOpts) }) + resultsPool.Init(func() index.Results { return index.NewResults(index.ResultsOptions{}, indexOpts) }) return opts.SetIndexOptions(indexOpts) } From c04b5e03dc30b3b8e29a82ce09aa6ceed676acd3 Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Thu, 21 Mar 2019 08:56:03 -0400 Subject: [PATCH 05/12] Fix unit tests --- .../tchannelthrift/node/service_test.go | 16 ++++++------- src/dbnode/server/server.go | 17 +++++++------ src/dbnode/storage/index/block_test.go | 24 +++++++++---------- src/dbnode/storage/index/options.go | 2 +- src/dbnode/storage/index/results.go | 7 +++++- src/dbnode/storage/index/results_test.go | 18 +++++++------- 6 files changed, 46 insertions(+), 38 deletions(-) diff --git a/src/dbnode/network/server/tchannelthrift/node/service_test.go b/src/dbnode/network/server/tchannelthrift/node/service_test.go index acdc0cd8a7..6bac1dce16 100644 --- a/src/dbnode/network/server/tchannelthrift/node/service_test.go +++ b/src/dbnode/network/server/tchannelthrift/node/service_test.go @@ -191,8 +191,8 @@ func TestServiceQuery(t *testing.T) { require.NoError(t, err) qry := index.Query{Query: req} - resMap := index.NewResults(testIndexOptions) - resMap.Reset(ident.StringID(nsID)) + resMap := index.NewResults(ident.StringID(nsID), + index.ResultsOptions{}, testIndexOptions) resMap.Map().Set(ident.StringID("foo"), ident.NewTags( ident.StringTag(tags["foo"][0].name, tags["foo"][0].value), ident.StringTag(tags["foo"][1].name, tags["foo"][1].value), @@ -1064,8 +1064,8 @@ func TestServiceFetchTagged(t *testing.T) { require.NoError(t, err) qry := index.Query{Query: req} - resMap := index.NewResults(testIndexOptions) - resMap.Reset(ident.StringID(nsID)) + resMap := index.NewResults(ident.StringID(nsID), + index.ResultsOptions{}, testIndexOptions) resMap.Map().Set(ident.StringID("foo"), ident.NewTags( ident.StringTag("foo", "bar"), ident.StringTag("baz", "dxk"), @@ -1161,8 +1161,8 @@ func TestServiceFetchTaggedIsOverloaded(t *testing.T) { req, err := idx.NewRegexpQuery([]byte("foo"), []byte("b.*")) require.NoError(t, err) - resMap := index.NewResults(testIndexOptions) - resMap.Reset(ident.StringID(nsID)) + resMap := index.NewResults(ident.StringID(nsID), + index.ResultsOptions{}, testIndexOptions) resMap.Map().Set(ident.StringID("foo"), ident.NewTags( ident.StringTag("foo", "bar"), ident.StringTag("baz", "dxk"), @@ -1214,8 +1214,8 @@ func TestServiceFetchTaggedNoData(t *testing.T) { require.NoError(t, err) qry := index.Query{Query: req} - resMap := index.NewResults(testIndexOptions) - resMap.Reset(ident.StringID(nsID)) + resMap := index.NewResults(ident.StringID(nsID), + index.ResultsOptions{}, testIndexOptions) resMap.Map().Set(ident.StringID("foo"), ident.Tags{}) resMap.Map().Set(ident.StringID("bar"), ident.Tags{}) mockDB.EXPECT().QueryIDs( diff --git a/src/dbnode/server/server.go b/src/dbnode/server/server.go index c04df075c5..ff183b9762 100644 --- a/src/dbnode/server/server.go +++ b/src/dbnode/server/server.go @@ -1199,12 +1199,11 @@ func withEncodingAndPoolingOptions( SetBytesPool(bytesPool). SetIdentifierPool(identifierPool)) - var ( - resultsPool = index.NewResultsPool( - poolOptions(policy.IndexResultsPool, scope.SubScope("index-results-pool"))) - postingsListOpts = poolOptions(policy.PostingsListPool, scope.SubScope("postingslist-pool")) - postingsList = postings.NewPool(postingsListOpts, roaring.NewPostingsList) - ) + postingsListOpts := poolOptions(policy.PostingsListPool, scope.SubScope("postingslist-pool")) + postingsList := postings.NewPool(postingsListOpts, roaring.NewPostingsList) + + resultsPool := index.NewResultsPool( + poolOptions(policy.IndexResultsPool, scope.SubScope("index-results-pool"))) indexOpts := opts.IndexOptions(). SetInstrumentOptions(iopts). @@ -1223,7 +1222,11 @@ func withEncodingAndPoolingOptions( SetCheckedBytesPool(bytesPool). SetResultsPool(resultsPool) - resultsPool.Init(func() index.Results { return index.NewResults(index.ResultsOptions{}, indexOpts) }) + resultsPool.Init(func() index.Results { + // NB(r): Need to initialize after setting the index opts so + // it seems the same reference of the options as is set + return index.NewResults(nil, index.ResultsOptions{}, indexOpts) + }) return opts.SetIndexOptions(indexOpts) } diff --git a/src/dbnode/storage/index/block_test.go b/src/dbnode/storage/index/block_test.go index 65fcb56db4..f048c08d36 100644 --- a/src/dbnode/storage/index/block_test.go +++ b/src/dbnode/storage/index/block_test.go @@ -481,7 +481,7 @@ func TestBlockMockQueryExecutorExecIterErr(t *testing.T) { exec.EXPECT().Close(), ) _, err = b.Query(resource.NewCancellableLifetime(), - Query{}, QueryOptions{}, NewResults(ResultsOptions{}, testOpts)) + Query{}, QueryOptions{}, NewResults(nil, ResultsOptions{}, testOpts)) require.Error(t, err) } @@ -513,7 +513,7 @@ func TestBlockMockQueryExecutorExecLimit(t *testing.T) { exec.EXPECT().Close().Return(nil), ) limit := 1 - results := NewResults(ResultsOptions{SizeLimit: limit}, testOpts) + results := NewResults(nil, ResultsOptions{SizeLimit: limit}, testOpts) exhaustive, err := b.Query(resource.NewCancellableLifetime(), Query{}, QueryOptions{Limit: limit}, results) require.NoError(t, err) @@ -553,7 +553,7 @@ func TestBlockMockQueryExecutorExecIterCloseErr(t *testing.T) { dIter.EXPECT().Close().Return(fmt.Errorf("random-err")), exec.EXPECT().Close().Return(nil), ) - results := NewResults(ResultsOptions{}, testOpts) + results := NewResults(nil, ResultsOptions{}, testOpts) _, err = b.Query(resource.NewCancellableLifetime(), Query{}, QueryOptions{}, results) require.Error(t, err) @@ -584,7 +584,7 @@ func TestBlockMockQueryExecutorExecIterExecCloseErr(t *testing.T) { dIter.EXPECT().Close().Return(nil), exec.EXPECT().Close().Return(fmt.Errorf("randomerr")), ) - results := NewResults(ResultsOptions{}, testOpts) + results := NewResults(nil, ResultsOptions{}, testOpts) _, err = b.Query(resource.NewCancellableLifetime(), Query{}, QueryOptions{}, results) require.Error(t, err) @@ -618,7 +618,7 @@ func TestBlockMockQueryLimit(t *testing.T) { exec.EXPECT().Close().Return(nil), ) limit := 1 - results := NewResults(ResultsOptions{SizeLimit: 1}, testOpts) + results := NewResults(nil, ResultsOptions{SizeLimit: 1}, testOpts) exhaustive, err := b.Query(resource.NewCancellableLifetime(), Query{}, QueryOptions{Limit: limit}, results) require.NoError(t, err) @@ -661,7 +661,7 @@ func TestBlockMockQueryLimitExhaustive(t *testing.T) { exec.EXPECT().Close().Return(nil), ) limit := 2 - results := NewResults(ResultsOptions{SizeLimit: limit}, testOpts) + results := NewResults(nil, ResultsOptions{SizeLimit: limit}, testOpts) exhaustive, err := b.Query(resource.NewCancellableLifetime(), Query{}, QueryOptions{Limit: limit}, results) require.NoError(t, err) @@ -695,7 +695,7 @@ func TestBlockMockQueryMergeResultsMapLimit(t *testing.T) { } limit := 1 - results := NewResults(ResultsOptions{SizeLimit: limit}, testOpts) + results := NewResults(nil, ResultsOptions{SizeLimit: limit}, testOpts) _, err = results.AddDocuments([]doc.Document{testDoc1()}) require.NoError(t, err) @@ -738,7 +738,7 @@ func TestBlockMockQueryMergeResultsDupeID(t *testing.T) { return exec, nil } - results := NewResults(ResultsOptions{}, testOpts) + results := NewResults(nil, ResultsOptions{}, testOpts) _, err = results.AddDocuments([]doc.Document{testDoc1()}) require.NoError(t, err) @@ -1155,7 +1155,7 @@ func TestBlockE2EInsertQuery(t *testing.T) { q, err := idx.NewRegexpQuery([]byte("bar"), []byte("b.*")) require.NoError(t, err) - results := NewResults(ResultsOptions{}, testOpts) + results := NewResults(nil, ResultsOptions{}, testOpts) exhaustive, err := b.Query(resource.NewCancellableLifetime(), Query{q}, QueryOptions{}, results) require.NoError(t, err) @@ -1224,7 +1224,7 @@ func TestBlockE2EInsertQueryLimit(t *testing.T) { require.NoError(t, err) limit := 1 - results := NewResults(ResultsOptions{SizeLimit: limit}, testOpts) + results := NewResults(nil, ResultsOptions{SizeLimit: limit}, testOpts) exhaustive, err := b.Query(resource.NewCancellableLifetime(), Query{q}, QueryOptions{Limit: limit}, results) require.NoError(t, err) @@ -1303,7 +1303,7 @@ func TestBlockE2EInsertAddResultsQuery(t *testing.T) { q, err := idx.NewRegexpQuery([]byte("bar"), []byte("b.*")) require.NoError(t, err) - results := NewResults(ResultsOptions{}, testOpts) + results := NewResults(nil, ResultsOptions{}, testOpts) exhaustive, err := b.Query(resource.NewCancellableLifetime(), Query{q}, QueryOptions{}, results) require.NoError(t, err) @@ -1367,7 +1367,7 @@ func TestBlockE2EInsertAddResultsMergeQuery(t *testing.T) { q, err := idx.NewRegexpQuery([]byte("bar"), []byte("b.*")) require.NoError(t, err) - results := NewResults(ResultsOptions{}, testOpts) + results := NewResults(nil, ResultsOptions{}, testOpts) exhaustive, err := b.Query(resource.NewCancellableLifetime(), Query{q}, QueryOptions{}, results) require.NoError(t, err) diff --git a/src/dbnode/storage/index/options.go b/src/dbnode/storage/index/options.go index 0ff34ad007..697f140e28 100644 --- a/src/dbnode/storage/index/options.go +++ b/src/dbnode/storage/index/options.go @@ -140,7 +140,7 @@ func NewOptions() Options { foregroundCompactionPlannerOpts: defaultForegroundCompactionOpts, backgroundCompactionPlannerOpts: defaultBackgroundCompactionOpts, } - resultsPool.Init(func() Results { return NewResults(ResultsOptions{}, opts) }) + resultsPool.Init(func() Results { return NewResults(nil, ResultsOptions{}, opts) }) return opts } diff --git a/src/dbnode/storage/index/results.go b/src/dbnode/storage/index/results.go index 774c2ac5fc..edafc44d54 100644 --- a/src/dbnode/storage/index/results.go +++ b/src/dbnode/storage/index/results.go @@ -50,8 +50,13 @@ type results struct { } // NewResults returns a new results object. -func NewResults(opts ResultsOptions, indexOpts Options) Results { +func NewResults( + namespaceID ident.ID, + opts ResultsOptions, + indexOpts Options, +) Results { return &results{ + nsID: namespaceID, opts: opts, resultsMap: newResultsMap(indexOpts.IdentifierPool()), idPool: indexOpts.IdentifierPool(), diff --git a/src/dbnode/storage/index/results_test.go b/src/dbnode/storage/index/results_test.go index 80ec90d6a0..19809be703 100644 --- a/src/dbnode/storage/index/results_test.go +++ b/src/dbnode/storage/index/results_test.go @@ -50,7 +50,7 @@ func init() { } func TestResultsInsertInvalid(t *testing.T) { - res := NewResults(ResultsOptions{}, testOpts) + res := NewResults(nil, ResultsOptions{}, testOpts) dInvalid := doc.Document{ID: nil} size, err := res.AddDocuments([]doc.Document{dInvalid}) require.Error(t, err) @@ -58,7 +58,7 @@ func TestResultsInsertInvalid(t *testing.T) { } func TestResultsInsertIdempotency(t *testing.T) { - res := NewResults(ResultsOptions{}, testOpts) + res := NewResults(nil, ResultsOptions{}, testOpts) dValid := doc.Document{ID: []byte("abc")} size, err := res.AddDocuments([]doc.Document{dValid}) require.NoError(t, err) @@ -70,7 +70,7 @@ func TestResultsInsertIdempotency(t *testing.T) { } func TestResultsFirstInsertWins(t *testing.T) { - res := NewResults(ResultsOptions{}, testOpts) + res := NewResults(nil, ResultsOptions{}, testOpts) d1 := doc.Document{ID: []byte("abc")} size, err := res.AddDocuments([]doc.Document{d1}) require.NoError(t, err) @@ -94,7 +94,7 @@ func TestResultsFirstInsertWins(t *testing.T) { } func TestResultsInsertContains(t *testing.T) { - res := NewResults(ResultsOptions{}, testOpts) + res := NewResults(nil, ResultsOptions{}, testOpts) dValid := doc.Document{ID: []byte("abc")} size, err := res.AddDocuments([]doc.Document{dValid}) require.NoError(t, err) @@ -106,7 +106,7 @@ func TestResultsInsertContains(t *testing.T) { } func TestResultsInsertCopies(t *testing.T) { - res := NewResults(ResultsOptions{}, testOpts) + res := NewResults(nil, ResultsOptions{}, testOpts) dValid := doc.Document{ID: []byte("abc"), Fields: []doc.Field{ doc.Field{Name: []byte("name"), Value: []byte("value")}, }} @@ -148,7 +148,7 @@ func TestResultsInsertCopies(t *testing.T) { } func TestResultsReset(t *testing.T) { - res := NewResults(ResultsOptions{}, testOpts) + res := NewResults(nil, ResultsOptions{}, testOpts) d1 := doc.Document{ID: []byte("abc")} size, err := res.AddDocuments([]doc.Document{d1}) require.NoError(t, err) @@ -166,7 +166,7 @@ func TestResultsReset(t *testing.T) { } func TestResultsResetNamespaceClones(t *testing.T) { - res := NewResults(ResultsOptions{}, testOpts) + res := NewResults(nil, ResultsOptions{}, testOpts) require.Equal(t, nil, res.Namespace()) nsID := ident.StringID("something") res.Reset(nsID, ResultsOptions{}) @@ -187,7 +187,7 @@ func tagsFromFields(fields []doc.Field) ident.Tags { func TestFinalize(t *testing.T) { // Create a Results and insert some data. - res := NewResults(ResultsOptions{}, testOpts) + res := NewResults(nil, ResultsOptions{}, testOpts) d1 := doc.Document{ID: []byte("abc")} size, err := res.AddDocuments([]doc.Document{d1}) require.NoError(t, err) @@ -217,7 +217,7 @@ func TestFinalize(t *testing.T) { func TestNoFinalize(t *testing.T) { // Create a Results and insert some data. - res := NewResults(ResultsOptions{}, testOpts) + res := NewResults(nil, ResultsOptions{}, testOpts) d1 := doc.Document{ID: []byte("abc")} size, err := res.AddDocuments([]doc.Document{d1}) require.NoError(t, err) From fa8331b79673a681914ea80b06ed863912bb7aca Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Thu, 21 Mar 2019 10:03:48 -0400 Subject: [PATCH 06/12] Fix prop test --- src/dbnode/storage/index/block_prop_test.go | 32 ++++++++++++++++++--- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/src/dbnode/storage/index/block_prop_test.go b/src/dbnode/storage/index/block_prop_test.go index cb8d88e330..5db1008483 100644 --- a/src/dbnode/storage/index/block_prop_test.go +++ b/src/dbnode/storage/index/block_prop_test.go @@ -38,6 +38,7 @@ import ( "github.com/m3db/m3/src/m3ninx/search" "github.com/m3db/m3/src/m3ninx/search/proptest" "github.com/m3db/m3/src/m3ninx/util" + "github.com/m3db/m3/src/x/resource" "github.com/m3db/m3x/instrument" "github.com/leanovate/gopter" @@ -107,8 +108,26 @@ func TestPostingsListCacheDoesNotAffectBlockQueryResults(t *testing.T) { idx.NewQueryFromSearchQuery(q), } - uncachedResults := NewResults(ResultsOptions{}, testOpts) - exhaustive, err := uncachedBlock.Query(indexQuery, QueryOptions{StartInclusive: blockStart, EndExclusive: blockStart.Add(blockSize)}, uncachedResults) + cancellable := resource.NewCancellableLifetime() + cancelled := false + doneQuery := func() { + if !cancelled { + cancelled = true + cancellable.Cancel() + } + } + + // In case we return early + defer doneQuery() + + queryOpts := QueryOptions{ + StartInclusive: blockStart, + EndExclusive: blockStart.Add(blockSize), + } + + uncachedResults := NewResults(nil, ResultsOptions{}, testOpts) + exhaustive, err := uncachedBlock.Query(cancellable, indexQuery, + queryOpts, uncachedResults) if err != nil { return false, fmt.Errorf("error querying uncached block: %v", err) } @@ -116,8 +135,9 @@ func TestPostingsListCacheDoesNotAffectBlockQueryResults(t *testing.T) { return false, errors.New("querying uncached block was not exhaustive") } - cachedResults := NewResults(ResultsOptions{}, testOpts) - exhaustive, err = cachedBlock.Query(indexQuery, QueryOptions{StartInclusive: blockStart, EndExclusive: blockStart.Add(blockSize)}, cachedResults) + cachedResults := NewResults(nil, ResultsOptions{}, testOpts) + exhaustive, err = cachedBlock.Query(cancellable, indexQuery, + queryOpts, cachedResults) if err != nil { return false, fmt.Errorf("error querying cached block: %v", err) } @@ -125,6 +145,10 @@ func TestPostingsListCacheDoesNotAffectBlockQueryResults(t *testing.T) { return false, errors.New("querying cached block was not exhaustive") } + // The lifetime of the query is complete, cancel the lifetime so we + // can safely access the results of each + doneQuery() + uncachedMap := uncachedResults.Map() cachedMap := cachedResults.Map() if uncachedMap.Len() != cachedMap.Len() { From 2f5db2dacf3d33c6debd24bb8aff71077b740c65 Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Thu, 21 Mar 2019 11:02:50 -0400 Subject: [PATCH 07/12] Fix metalint and small limit for quorum fetch tagged tests --- src/dbnode/integration/fetch_tagged_quorum_test.go | 2 +- src/dbnode/storage/index/results.go | 3 +-- src/dbnode/storage/index/results_test.go | 11 ----------- src/x/resource/lifetime.go | 1 - 4 files changed, 2 insertions(+), 15 deletions(-) diff --git a/src/dbnode/integration/fetch_tagged_quorum_test.go b/src/dbnode/integration/fetch_tagged_quorum_test.go index 22e64947a8..3bb6c46647 100644 --- a/src/dbnode/integration/fetch_tagged_quorum_test.go +++ b/src/dbnode/integration/fetch_tagged_quorum_test.go @@ -295,7 +295,7 @@ func makeTestFetchTagged( index.QueryOptions{ StartInclusive: startTime.Add(-time.Minute), EndExclusive: startTime.Add(time.Minute), - Limit: 1, + Limit: 100, }) } diff --git a/src/dbnode/storage/index/results.go b/src/dbnode/storage/index/results.go index edafc44d54..062fe22f33 100644 --- a/src/dbnode/storage/index/results.go +++ b/src/dbnode/storage/index/results.go @@ -30,8 +30,7 @@ import ( ) var ( - errUnableToAddResultMissingID = errors.New("no id for result") - errResultAlreadyExistsNoPartialUpdates = errors.New("id already exists for result and partial updates not allowed") + errUnableToAddResultMissingID = errors.New("no id for result") ) type results struct { diff --git a/src/dbnode/storage/index/results_test.go b/src/dbnode/storage/index/results_test.go index 19809be703..72d9dc692e 100644 --- a/src/dbnode/storage/index/results_test.go +++ b/src/dbnode/storage/index/results_test.go @@ -174,17 +174,6 @@ func TestResultsResetNamespaceClones(t *testing.T) { require.Equal(t, "something", res.Namespace().String()) } -func tagsFromFields(fields []doc.Field) ident.Tags { - tags := ident.NewTags() - for _, field := range fields { - tags.Append(ident.Tag{ - Name: ident.BytesID(field.Name), - Value: ident.BytesID(field.Value), - }) - } - return tags -} - func TestFinalize(t *testing.T) { // Create a Results and insert some data. res := NewResults(nil, ResultsOptions{}, testOpts) diff --git a/src/x/resource/lifetime.go b/src/x/resource/lifetime.go index 92319c673a..ef5c53baa1 100644 --- a/src/x/resource/lifetime.go +++ b/src/x/resource/lifetime.go @@ -27,7 +27,6 @@ import "sync" // cancelled will not allow any further checkouts. type CancellableLifetime struct { mu sync.RWMutex - checkouts int cancelled bool } From fe32fc27842c442b42b2aa7549ff530e5f640243 Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Thu, 21 Mar 2019 13:24:08 -0400 Subject: [PATCH 08/12] Add ability to mmap docs data for foreground/background compaction --- src/dbnode/storage/index/block.go | 30 ++++--- src/dbnode/storage/index/block_bench_test.go | 3 +- src/dbnode/storage/index/block_prop_test.go | 2 +- src/dbnode/storage/index/block_test.go | 85 ++++++++++--------- .../storage/index/compaction/compactor.go | 42 ++++++++- .../index/compaction/compactor_test.go | 25 ++++++ src/dbnode/storage/index/results_test.go | 14 +-- 7 files changed, 142 insertions(+), 59 deletions(-) diff --git a/src/dbnode/storage/index/block.go b/src/dbnode/storage/index/block.go index 43623f2bfe..94b99ec030 100644 --- a/src/dbnode/storage/index/block.go +++ b/src/dbnode/storage/index/block.go @@ -160,19 +160,26 @@ type blockShardRangesSegments struct { segments []segment.Segment } +// BlockOptions is a set of options used when constructing an index block. +type BlockOptions struct { + ForegroundCompactorMmapDocsData bool + BackgroundCompactorMmapDocsData bool +} + // NewBlock returns a new Block, representing a complete reverse index for the // duration of time specified. It is backed by one or more segments. func NewBlock( blockStart time.Time, md namespace.Metadata, - opts Options, + opts BlockOptions, + indexOpts Options, ) (Block, error) { - docsPool := opts.DocumentArrayPool() + docsPool := indexOpts.DocumentArrayPool() foregroundCompactor := compaction.NewCompactor(docsPool, documentArrayPoolCapacity, - opts.SegmentBuilderOptions(), - opts.FSTSegmentOptions(), + indexOpts.SegmentBuilderOptions(), + indexOpts.FSTSegmentOptions(), compaction.CompactorOptions{ FSTWriterOptions: &fst.WriterOptions{ // DisableRegistry is set to true to trade a larger FST size @@ -180,28 +187,31 @@ func NewBlock( // to end latency for time to first index a metric. DisableRegistry: true, }, + MmapDocsData: opts.BackgroundCompactorMmapDocsData, }) backgroundCompactor := compaction.NewCompactor(docsPool, documentArrayPoolCapacity, - opts.SegmentBuilderOptions(), - opts.FSTSegmentOptions(), - compaction.CompactorOptions{}) + indexOpts.SegmentBuilderOptions(), + indexOpts.FSTSegmentOptions(), + compaction.CompactorOptions{ + MmapDocsData: opts.ForegroundCompactorMmapDocsData, + }) - segmentBuilder, err := builder.NewBuilderFromDocuments(opts.SegmentBuilderOptions()) + segmentBuilder, err := builder.NewBuilderFromDocuments(indexOpts.SegmentBuilderOptions()) if err != nil { return nil, err } blockSize := md.Options().IndexOptions().BlockSize() - iopts := opts.InstrumentOptions() + iopts := indexOpts.InstrumentOptions() b := &block{ state: blockStateOpen, segmentBuilder: segmentBuilder, blockStart: blockStart, blockEnd: blockStart.Add(blockSize), blockSize: blockSize, - opts: opts, + opts: indexOpts, iopts: iopts, nsMD: md, docsPool: docsPool, diff --git a/src/dbnode/storage/index/block_bench_test.go b/src/dbnode/storage/index/block_bench_test.go index a941e239b5..6c493510fb 100644 --- a/src/dbnode/storage/index/block_bench_test.go +++ b/src/dbnode/storage/index/block_bench_test.go @@ -46,7 +46,8 @@ func BenchmarkBlockWrite(b *testing.B) { now := time.Now() blockStart := now.Truncate(blockSize) - bl, err := NewBlock(blockStart, testMD, testOpts) + bl, err := NewBlock(blockStart, testMD, + BlockOptions{}, testOpts) require.NoError(b, err) defer func() { require.NoError(b, bl.Close()) diff --git a/src/dbnode/storage/index/block_prop_test.go b/src/dbnode/storage/index/block_prop_test.go index 5db1008483..ee883096dd 100644 --- a/src/dbnode/storage/index/block_prop_test.go +++ b/src/dbnode/storage/index/block_prop_test.go @@ -179,7 +179,7 @@ func TestPostingsListCacheDoesNotAffectBlockQueryResults(t *testing.T) { } func newPropTestBlock(t *testing.T, blockStart time.Time, nsMeta namespace.Metadata, opts Options) (Block, error) { - blk, err := NewBlock(blockStart, nsMeta, opts) + blk, err := NewBlock(blockStart, nsMeta, BlockOptions{}, opts) require.NoError(t, err) var ( diff --git a/src/dbnode/storage/index/block_test.go b/src/dbnode/storage/index/block_test.go index f048c08d36..a039668bcb 100644 --- a/src/dbnode/storage/index/block_test.go +++ b/src/dbnode/storage/index/block_test.go @@ -60,7 +60,7 @@ func newTestNSMetadata(t require.TestingT) namespace.Metadata { func TestBlockCtor(t *testing.T) { md := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - b, err := NewBlock(start, md, testOpts) + b, err := NewBlock(start, md, BlockOptions{}, testOpts) require.NoError(t, err) require.Equal(t, start, b.StartTime()) @@ -83,7 +83,7 @@ func TestBlockWriteAfterClose(t *testing.T) { Truncate(blockSize). Add(time.Minute) - b, err := NewBlock(blockStart, testMD, testOpts) + b, err := NewBlock(blockStart, testMD, BlockOptions{}, testOpts) require.NoError(t, err) require.NoError(t, b.Close()) @@ -131,7 +131,7 @@ func TestBlockWriteAfterSeal(t *testing.T) { Truncate(blockSize). Add(time.Minute) - b, err := NewBlock(blockStart, testMD, testOpts) + b, err := NewBlock(blockStart, testMD, BlockOptions{}, testOpts) require.NoError(t, err) require.NoError(t, b.Seal()) @@ -179,7 +179,7 @@ func TestBlockWrite(t *testing.T) { Truncate(blockSize). Add(time.Minute) - blk, err := NewBlock(blockStart, testMD, testOpts) + blk, err := NewBlock(blockStart, testMD, BlockOptions{}, testOpts) require.NoError(t, err) defer func() { require.NoError(t, blk.Close()) @@ -228,7 +228,7 @@ func TestBlockWriteActualSegmentPartialFailure(t *testing.T) { Truncate(blockSize). Add(time.Minute) - blk, err := NewBlock(blockStart, md, testOpts) + blk, err := NewBlock(blockStart, md, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) require.True(t, ok) @@ -288,7 +288,7 @@ func TestBlockWritePartialFailure(t *testing.T) { Truncate(blockSize). Add(time.Minute) - blk, err := NewBlock(blockStart, md, testOpts) + blk, err := NewBlock(blockStart, md, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) require.True(t, ok) @@ -337,7 +337,7 @@ func TestBlockWritePartialFailure(t *testing.T) { func TestBlockQueryAfterClose(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - b, err := NewBlock(start, testMD, testOpts) + b, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) require.Equal(t, start, b.StartTime()) @@ -352,7 +352,7 @@ func TestBlockQueryAfterClose(t *testing.T) { func TestBlockQueryExecutorError(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -375,7 +375,7 @@ func TestBlockQuerySegmentReaderError(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -397,7 +397,7 @@ func TestBlockQueryAddResultsSegmentsError(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -433,7 +433,7 @@ func TestBlockMockQueryExecutorExecError(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -459,7 +459,7 @@ func TestBlockMockQueryExecutorExecIterErr(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -491,7 +491,7 @@ func TestBlockMockQueryExecutorExecLimit(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -534,7 +534,7 @@ func TestBlockMockQueryExecutorExecIterCloseErr(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -565,7 +565,7 @@ func TestBlockMockQueryExecutorExecIterExecCloseErr(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -596,7 +596,7 @@ func TestBlockMockQueryLimit(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -639,7 +639,7 @@ func TestBlockMockQueryLimitExhaustive(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -682,7 +682,7 @@ func TestBlockMockQueryMergeResultsMapLimit(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -727,7 +727,7 @@ func TestBlockMockQueryMergeResultsDupeID(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -780,7 +780,7 @@ func TestBlockAddResultsAddsSegment(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -801,7 +801,7 @@ func TestBlockAddResultsAfterCloseFails(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) require.NoError(t, blk.Close()) @@ -817,7 +817,7 @@ func TestBlockAddResultsAfterSealWorks(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) require.NoError(t, blk.Seal()) @@ -839,7 +839,7 @@ func TestBlockTickSingleSegment(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -861,7 +861,7 @@ func TestBlockTickMultipleSegment(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -889,7 +889,7 @@ func TestBlockTickAfterSeal(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) require.NoError(t, blk.Seal()) @@ -912,7 +912,7 @@ func TestBlockTickAfterClose(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) require.NoError(t, blk.Close()) @@ -926,7 +926,7 @@ func TestBlockAddResultsRangeCheck(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -947,7 +947,7 @@ func TestBlockAddResultsCoversCurrentData(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -975,7 +975,7 @@ func TestBlockAddResultsDoesNotCoverCurrentData(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -1004,7 +1004,7 @@ func TestBlockNeedsMutableSegmentsEvicted(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -1038,7 +1038,7 @@ func TestBlockNeedsMutableSegmentsEvictedMutableSegments(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -1068,7 +1068,7 @@ func TestBlockEvictMutableSegmentsSimple(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) err = blk.EvictMutableSegments() require.Error(t, err) @@ -1084,7 +1084,7 @@ func TestBlockEvictMutableSegmentsAddResults(t *testing.T) { testMD := newTestNSMetadata(t) start := time.Now().Truncate(time.Hour) - blk, err := NewBlock(start, testMD, testOpts) + blk, err := NewBlock(start, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) @@ -1123,7 +1123,16 @@ func TestBlockE2EInsertQuery(t *testing.T) { Truncate(blockSize). Add(time.Minute) - blk, err := NewBlock(blockStart, testMD, testOpts) + // Use a larger batch size to simulate large number in a batch + // coming back (to ensure code path for reusing buffers for iterator + // is covered). + testOpts := optionsWithDocsArrayPool(testOpts, 16, 256) + + blk, err := NewBlock(blockStart, testMD, + BlockOptions{ + ForegroundCompactorMmapDocsData: true, + BackgroundCompactorMmapDocsData: true, + }, testOpts) require.NoError(t, err) b, ok := blk.(*block) require.True(t, ok) @@ -1190,7 +1199,7 @@ func TestBlockE2EInsertQueryLimit(t *testing.T) { Truncate(blockSize). Add(time.Minute) - blk, err := NewBlock(blockStart, testMD, testOpts) + blk, err := NewBlock(blockStart, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) require.True(t, ok) @@ -1266,7 +1275,7 @@ func TestBlockE2EInsertAddResultsQuery(t *testing.T) { Truncate(blockSize). Add(time.Minute) - blk, err := NewBlock(blockStart, testMD, testOpts) + blk, err := NewBlock(blockStart, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) require.True(t, ok) @@ -1338,7 +1347,7 @@ func TestBlockE2EInsertAddResultsMergeQuery(t *testing.T) { Truncate(blockSize). Add(time.Minute) - blk, err := NewBlock(blockStart, testMD, testOpts) + blk, err := NewBlock(blockStart, testMD, BlockOptions{}, testOpts) require.NoError(t, err) b, ok := blk.(*block) require.True(t, ok) @@ -1406,7 +1415,7 @@ func TestBlockWriteBackgroundCompact(t *testing.T) { testOpts.InstrumentOptions(). SetLogger(xlog.NewLevelLogger(xlog.SimpleLogger, xlog.LevelDebug))) - blk, err := NewBlock(blockStart, testMD, testOpts) + blk, err := NewBlock(blockStart, testMD, BlockOptions{}, testOpts) require.NoError(t, err) defer func() { require.NoError(t, blk.Close()) diff --git a/src/dbnode/storage/index/compaction/compactor.go b/src/dbnode/storage/index/compaction/compactor.go index c8c22140da..a83980b6b5 100644 --- a/src/dbnode/storage/index/compaction/compactor.go +++ b/src/dbnode/storage/index/compaction/compactor.go @@ -45,6 +45,7 @@ var ( type Compactor struct { sync.RWMutex + opts CompactorOptions writer fst.Writer docsPool doc.DocumentArrayPool docsMaxBatch int @@ -59,6 +60,12 @@ type CompactorOptions struct { // FSTWriterOptions if not nil are the options used to // construct the FST writer. FSTWriterOptions *fst.WriterOptions + + // MmapDocsData when enabled will encode and mmmap the + // documents data, rather than keeping the original + // documents with references to substrings in the metric + // IDs (done for memory savings). + MmapDocsData bool } // NewCompactor returns a new compactor which reuses buffers @@ -75,6 +82,7 @@ func NewCompactor( fstWriterOpts = *v } return &Compactor{ + opts: opts, writer: fst.NewWriter(fstWriterOpts), docsPool: docsPool, docsMaxBatch: docsMaxBatch, @@ -225,9 +233,6 @@ func (c *Compactor) compactFromBuilderWithLock( return nil, errCompactorBuilderEmpty } - allDocsCopy := make([]doc.Document, len(allDocs)) - copy(allDocsCopy, allDocs) - err := c.writer.Reset(builder) if err != nil { return nil, err @@ -239,7 +244,6 @@ func (c *Compactor) compactFromBuilderWithLock( MajorVersion: c.writer.MajorVersion(), MinorVersion: c.writer.MinorVersion(), Metadata: append([]byte(nil), c.writer.Metadata()...), - DocsReader: docs.NewSliceReader(0, allDocsCopy), Closer: closers, } @@ -250,6 +254,36 @@ func (c *Compactor) compactFromBuilderWithLock( } }() + if !c.opts.MmapDocsData { + // If retaining references to the original docs, simply take ownership + // of the documents and then reference them directly from the FST segment + // rather than encoding them and mmap'ing the encoded documents. + allDocsCopy := make([]doc.Document, len(allDocs)) + copy(allDocsCopy, allDocs) + fstData.DocsReader = docs.NewSliceReader(0, allDocsCopy) + } else { + // Otherwise encode and reference the encoded bytes as mmap'd bytes. + c.buff.Reset() + if err := c.writer.WriteDocumentsData(c.buff); err != nil { + return nil, err + } + + fstData.DocsData, err = c.mmapAndAppendCloser(c.buff.Bytes(), closers) + if err != nil { + return nil, err + } + + c.buff.Reset() + if err := c.writer.WriteDocumentsIndex(c.buff); err != nil { + return nil, err + } + + fstData.DocsIdxData, err = c.mmapAndAppendCloser(c.buff.Bytes(), closers) + if err != nil { + return nil, err + } + } + c.buff.Reset() if err := c.writer.WritePostingsOffsets(c.buff); err != nil { return nil, err diff --git a/src/dbnode/storage/index/compaction/compactor_test.go b/src/dbnode/storage/index/compaction/compactor_test.go index 3fb57c219c..8d267f9d3b 100644 --- a/src/dbnode/storage/index/compaction/compactor_test.go +++ b/src/dbnode/storage/index/compaction/compactor_test.go @@ -102,6 +102,31 @@ func TestCompactorSingleMutableSegment(t *testing.T) { require.NoError(t, compactor.Close()) } +func TestCompactorSingleMutableSegmentWithMmapDocsData(t *testing.T) { + seg, err := mem.NewSegment(0, testMemSegmentOptions) + require.NoError(t, err) + + _, err = seg.Insert(testDocuments[0]) + require.NoError(t, err) + + _, err = seg.Insert(testDocuments[1]) + require.NoError(t, err) + + compactor := NewCompactor(testDocsPool, testDocsMaxBatch, + testBuilderSegmentOptions, testFSTSegmentOptions, CompactorOptions{ + MmapDocsData: true, + }) + + compacted, err := compactor.Compact([]segment.Segment{ + mustSeal(t, seg), + }) + require.NoError(t, err) + + assertContents(t, compacted, testDocuments) + + require.NoError(t, compactor.Close()) +} + func TestCompactorManySegments(t *testing.T) { seg1, err := mem.NewSegment(0, testMemSegmentOptions) require.NoError(t, err) diff --git a/src/dbnode/storage/index/results_test.go b/src/dbnode/storage/index/results_test.go index 72d9dc692e..8802d7893d 100644 --- a/src/dbnode/storage/index/results_test.go +++ b/src/dbnode/storage/index/results_test.go @@ -38,15 +38,19 @@ var ( ) func init() { - // Use size of 1 to test edge cases + // Use size of 1 to test edge cases by default + testOpts = optionsWithDocsArrayPool(NewOptions(), 1, 1) +} + +func optionsWithDocsArrayPool(opts Options, size, capacity int) Options { docArrayPool := doc.NewDocumentArrayPool(doc.DocumentArrayPoolOpts{ - Options: pool.NewObjectPoolOptions().SetSize(1), - Capacity: 1, - MaxCapacity: 1, + Options: pool.NewObjectPoolOptions().SetSize(size), + Capacity: capacity, + MaxCapacity: capacity, }) docArrayPool.Init() - testOpts = NewOptions().SetDocumentArrayPool(docArrayPool) + return opts.SetDocumentArrayPool(docArrayPool) } func TestResultsInsertInvalid(t *testing.T) { From 534de744915c2b138dc6c0948d85c04737799671 Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Thu, 21 Mar 2019 13:25:18 -0400 Subject: [PATCH 09/12] Fix comment typos --- src/dbnode/server/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dbnode/server/server.go b/src/dbnode/server/server.go index ff183b9762..8fe5722bf3 100644 --- a/src/dbnode/server/server.go +++ b/src/dbnode/server/server.go @@ -1224,7 +1224,7 @@ func withEncodingAndPoolingOptions( resultsPool.Init(func() index.Results { // NB(r): Need to initialize after setting the index opts so - // it seems the same reference of the options as is set + // it sees the same reference of the options as is set for the DB. return index.NewResults(nil, index.ResultsOptions{}, indexOpts) }) From 98205b502d7516aa82bf0378acdcbc3ed36af8ee Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Thu, 21 Mar 2019 13:45:01 -0400 Subject: [PATCH 10/12] Fix NewBlock build errors --- src/dbnode/storage/index.go | 10 ++++- src/dbnode/storage/index_block_test.go | 56 ++++++++++++++++++++++---- 2 files changed, 56 insertions(+), 10 deletions(-) diff --git a/src/dbnode/storage/index.go b/src/dbnode/storage/index.go index a4b1cc52b5..eb5e4de0b7 100644 --- a/src/dbnode/storage/index.go +++ b/src/dbnode/storage/index.go @@ -142,7 +142,12 @@ type nsIndexRuntimeOptions struct { defaultQueryTimeout time.Duration } -type newBlockFn func(time.Time, namespace.Metadata, index.Options) (index.Block, error) +type newBlockFn func( + time.Time, + namespace.Metadata, + index.BlockOptions, + index.Options, +) (index.Block, error) // NB(prateek): the returned filesets are strictly before the given time, i.e. they // live in the period (-infinity, exclusiveTime). @@ -1137,7 +1142,8 @@ func (i *nsIndex) ensureBlockPresentWithRLock(blockStart time.Time) (index.Block } // ok now we know for sure we have to alloc - block, err := i.newBlockFn(blockStart, i.nsMetadata, i.opts.IndexOptions()) + block, err := i.newBlockFn(blockStart, i.nsMetadata, + index.BlockOptions{}, i.opts.IndexOptions()) if err != nil { // unable to allocate the block, should never happen. return nil, i.unableToAllocBlockInvariantError(err) } diff --git a/src/dbnode/storage/index_block_test.go b/src/dbnode/storage/index_block_test.go index c303727c51..066f73dff9 100644 --- a/src/dbnode/storage/index_block_test.go +++ b/src/dbnode/storage/index_block_test.go @@ -115,7 +115,12 @@ func TestNamespaceIndexNewBlockFn(t *testing.T) { mockBlock := index.NewMockBlock(ctrl) mockBlock.EXPECT().Stats(gomock.Any()).Return(nil).AnyTimes() mockBlock.EXPECT().Close().Return(nil) - newBlockFn := func(ts time.Time, md namespace.Metadata, io index.Options) (index.Block, error) { + newBlockFn := func( + ts time.Time, + md namespace.Metadata, + _ index.BlockOptions, + io index.Options, + ) (index.Block, error) { require.Equal(t, now.Truncate(blockSize), ts) return mockBlock, nil } @@ -152,7 +157,12 @@ func TestNamespaceIndexNewBlockFnRandomErr(t *testing.T) { opts := testDatabaseOptions() opts = opts.SetClockOptions(opts.ClockOptions().SetNowFn(nowFn)) - newBlockFn := func(ts time.Time, md namespace.Metadata, io index.Options) (index.Block, error) { + newBlockFn := func( + ts time.Time, + md namespace.Metadata, + _ index.BlockOptions, + io index.Options, + ) (index.Block, error) { return nil, fmt.Errorf("randomerr") } md := testNamespaceMetadata(blockSize, 4*time.Hour) @@ -174,7 +184,12 @@ func TestNamespaceIndexWrite(t *testing.T) { mockBlock.EXPECT().Stats(gomock.Any()).Return(nil).AnyTimes() mockBlock.EXPECT().Close().Return(nil) mockBlock.EXPECT().StartTime().Return(now.Truncate(blockSize)).AnyTimes() - newBlockFn := func(ts time.Time, md namespace.Metadata, io index.Options) (index.Block, error) { + newBlockFn := func( + ts time.Time, + md namespace.Metadata, + _ index.BlockOptions, + io index.Options, + ) (index.Block, error) { require.Equal(t, now.Truncate(blockSize), ts) return mockBlock, nil } @@ -237,7 +252,12 @@ func TestNamespaceIndexWriteCreatesBlock(t *testing.T) { b1.EXPECT().Stats(gomock.Any()).Return(nil).AnyTimes() b1.EXPECT().Close().Return(nil) b1.EXPECT().StartTime().Return(t1).AnyTimes() - newBlockFn := func(ts time.Time, md namespace.Metadata, io index.Options) (index.Block, error) { + newBlockFn := func( + ts time.Time, + md namespace.Metadata, + _ index.BlockOptions, + io index.Options, + ) (index.Block, error) { if ts.Equal(t0) { return b0, nil } @@ -309,7 +329,12 @@ func TestNamespaceIndexBootstrap(t *testing.T) { b1 := index.NewMockBlock(ctrl) b1.EXPECT().Stats(gomock.Any()).Return(nil).AnyTimes() b1.EXPECT().StartTime().Return(t1).AnyTimes() - newBlockFn := func(ts time.Time, md namespace.Metadata, io index.Options) (index.Block, error) { + newBlockFn := func( + ts time.Time, + md namespace.Metadata, + _ index.BlockOptions, + io index.Options, + ) (index.Block, error) { if ts.Equal(t0) { return b0, nil } @@ -355,7 +380,12 @@ func TestNamespaceIndexTickExpire(t *testing.T) { b0 := index.NewMockBlock(ctrl) b0.EXPECT().Stats(gomock.Any()).Return(nil).AnyTimes() b0.EXPECT().StartTime().Return(t0).AnyTimes() - newBlockFn := func(ts time.Time, md namespace.Metadata, io index.Options) (index.Block, error) { + newBlockFn := func( + ts time.Time, + md namespace.Metadata, + _ index.BlockOptions, + io index.Options, + ) (index.Block, error) { if ts.Equal(t0) { return b0, nil } @@ -399,7 +429,12 @@ func TestNamespaceIndexTick(t *testing.T) { b0.EXPECT().Stats(gomock.Any()).Return(nil).AnyTimes() b0.EXPECT().Close().Return(nil) b0.EXPECT().StartTime().Return(t0).AnyTimes() - newBlockFn := func(ts time.Time, md namespace.Metadata, io index.Options) (index.Block, error) { + newBlockFn := func( + ts time.Time, + md namespace.Metadata, + _ index.BlockOptions, + io index.Options, + ) (index.Block, error) { if ts.Equal(t0) { return b0, nil } @@ -490,7 +525,12 @@ func TestNamespaceIndexBlockQuery(t *testing.T) { b1.EXPECT().Close().Return(nil) b1.EXPECT().StartTime().Return(t1).AnyTimes() b1.EXPECT().EndTime().Return(t1.Add(blockSize)).AnyTimes() - newBlockFn := func(ts time.Time, md namespace.Metadata, io index.Options) (index.Block, error) { + newBlockFn := func( + ts time.Time, + md namespace.Metadata, + _ index.BlockOptions, + io index.Options, + ) (index.Block, error) { if ts.Equal(t0) { return b0, nil } From ff46b459b669c7458d44f1a24d2d989a4898e05a Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Thu, 21 Mar 2019 16:35:30 -0400 Subject: [PATCH 11/12] Address feedback --- src/dbnode/storage/index.go | 23 +++++++++++++---------- src/dbnode/storage/index/block.go | 6 ++++-- src/dbnode/storage/index/results.go | 18 ++++++++---------- src/dbnode/storage/index/types.go | 8 ++++++-- src/x/resource/lifetime.go | 8 +++++++- 5 files changed, 38 insertions(+), 25 deletions(-) diff --git a/src/dbnode/storage/index.go b/src/dbnode/storage/index.go index eb5e4de0b7..6618af8005 100644 --- a/src/dbnode/storage/index.go +++ b/src/dbnode/storage/index.go @@ -932,10 +932,11 @@ func (i *nsIndex) Query( return } - // If block had more data but we stopped early, need to notify caller. if blockExhaustive { return } + + // If block had more data but we stopped early, need to notify caller. state.exhaustive = false } @@ -943,17 +944,20 @@ func (i *nsIndex) Query( // Capture block for async query execution below. block := block - // Terminate early if we know we don't need any more results. + // We're looping through all the blocks that we need to query and kicking + // off parallel queries which are bounded by the queryWorkersPool's maximum + // concurrency. This means that it's possible at this point that we've + // completed querying one or more blocks and already exhausted the maximum + // number of results that we're allowed to return. If thats the case, there + // is no value in kicking off more parallel queries, so we break out of + // the loop. size := results.Size() - alreadyNotExhaustive := opts.LimitExceeded(size) - if alreadyNotExhaustive { + alreadyExceededLimit := opts.LimitExceeded(size) + if alreadyExceededLimit { state.Lock() state.exhaustive = false state.Unlock() - } - - if alreadyNotExhaustive { - // Break out if already exhaustive. + // Break out if already not exhaustive. break } @@ -1025,8 +1029,7 @@ func (i *nsIndex) Query( } state.Lock() - // Take reference to vars to return while locked, need to allow defer - // lock/unlock cleanup to not deadlock with this locked code block. + // Take reference to vars to return while locked. exhaustive := state.exhaustive err = state.multiErr.FinalError() state.Unlock() diff --git a/src/dbnode/storage/index/block.go b/src/dbnode/storage/index/block.go index 94b99ec030..ccda78bc8b 100644 --- a/src/dbnode/storage/index/block.go +++ b/src/dbnode/storage/index/block.go @@ -187,7 +187,7 @@ func NewBlock( // to end latency for time to first index a metric. DisableRegistry: true, }, - MmapDocsData: opts.BackgroundCompactorMmapDocsData, + MmapDocsData: opts.ForegroundCompactorMmapDocsData, }) backgroundCompactor := compaction.NewCompactor(docsPool, @@ -195,7 +195,7 @@ func NewBlock( indexOpts.SegmentBuilderOptions(), indexOpts.FSTSegmentOptions(), compaction.CompactorOptions{ - MmapDocsData: opts.ForegroundCompactorMmapDocsData, + MmapDocsData: opts.BackgroundCompactorMmapDocsData, }) segmentBuilder, err := builder.NewBuilderFromDocuments(indexOpts.SegmentBuilderOptions()) @@ -762,6 +762,7 @@ func (b *block) executorWithRLock() (search.Executor, error) { return executor.NewExecutor(readers), nil } +// Query guarentees func (b *block) Query( cancellable *resource.CancellableLifetime, query Query, @@ -770,6 +771,7 @@ func (b *block) Query( ) (bool, error) { b.RLock() defer b.RUnlock() + if b.state == blockStateClosed { return false, ErrUnableToQueryBlockClosed } diff --git a/src/dbnode/storage/index/results.go b/src/dbnode/storage/index/results.go index 062fe22f33..f48e554f77 100644 --- a/src/dbnode/storage/index/results.go +++ b/src/dbnode/storage/index/results.go @@ -69,23 +69,23 @@ func (r *results) Reset(nsID ident.ID, opts ResultsOptions) { r.opts = opts - // finalize existing held nsID + // Finalize existing held nsID. if r.nsID != nil { r.nsID.Finalize() } - // make an independent copy of the new nsID + // Make an independent copy of the new nsID. if nsID != nil { nsID = r.idPool.Clone(nsID) } r.nsID = nsID - // reset all values from map first + // Reset all values from map first for _, entry := range r.resultsMap.Iter() { tags := entry.Value() tags.Finalize() } - // reset all keys in the map next + // Reset all keys in the map next, this will finalize the keys. r.resultsMap.Reset() // NB: could do keys+value in one step but I'm trying to avoid @@ -109,7 +109,7 @@ func (r *results) addDocumentsBatchWithLock(batch []doc.Document) error { return err } if r.opts.SizeLimit > 0 && size >= r.opts.SizeLimit { - // Early return if limit enforced and we hit our limit + // Early return if limit enforced and we hit our limit. break } } @@ -119,9 +119,8 @@ func (r *results) addDocumentsBatchWithLock(batch []doc.Document) error { func (r *results) addDocumentWithLock( d doc.Document, ) (bool, int, error) { - added := false if len(d.ID) == 0 { - return added, r.resultsMap.Len(), errUnableToAddResultMissingID + return false, r.resultsMap.Len(), errUnableToAddResultMissingID } // NB: can cast the []byte -> ident.ID to avoid an alloc @@ -130,7 +129,7 @@ func (r *results) addDocumentWithLock( // check if it already exists in the map. if r.resultsMap.Contains(tsID) { - return added, r.resultsMap.Len(), nil + return false, r.resultsMap.Len(), nil } // i.e. it doesn't exist in the map, so we create the tags wrapping @@ -141,8 +140,7 @@ func (r *results) addDocumentWithLock( // the tsID's bytes. r.resultsMap.Set(tsID, tags) - added = true - return added, r.resultsMap.Len(), nil + return true, r.resultsMap.Len(), nil } func (r *results) cloneTagsFromFields(fields doc.Fields) ident.Tags { diff --git a/src/dbnode/storage/index/types.go b/src/dbnode/storage/index/types.go index ba107ccaba..ad4e37d1ee 100644 --- a/src/dbnode/storage/index/types.go +++ b/src/dbnode/storage/index/types.go @@ -82,8 +82,6 @@ type QueryResults struct { // Results is a collection of results for a query, it is synchronized // when access to the results set is used as documented by the methods. -// It cannot be written to after it is sealed, until it's reopened by -// resetting the results. type Results interface { // Reset resets the Results object to initial state. Reset(nsID ident.ID, opts ResultsOptions) @@ -96,6 +94,10 @@ type Results interface { // Map returns the results map from seriesID -> seriesTags, comprising // index results. + // Since a lock is not held when accessing the map after a call to this + // method it is not safe to read or write to the map if any other caller + // mutates the state of the results after obtainin a reference to the map + // with this call. Map() *ResultsMap // AddDocuments adds the batch of documents to the results set, it will @@ -103,6 +105,8 @@ type Results interface { // modified after this function returns without affecting the results map. // If documents with duplicate IDs are added, they are simply ignored and // the first document added with an ID is returned. + // TODO(r): We will need to change this behavior once index fields are + // mutable and the most recent need to shadow older entries. AddDocuments(batch []doc.Document) (size int, err error) // Finalize releases any resources held by the Results object, diff --git a/src/x/resource/lifetime.go b/src/x/resource/lifetime.go index ef5c53baa1..7e1fcb10fe 100644 --- a/src/x/resource/lifetime.go +++ b/src/x/resource/lifetime.go @@ -39,6 +39,9 @@ func NewCancellableLifetime() *CancellableLifetime { // is already cancelled this will return false, otherwise it will return // true and guarantee the lifetime is not cancelled until the checkout // is returned. +// If this returns true you MUST call ReleaseCheckout later, otherwise +// the lifetime will never close and any caller calling Cancel will be +// blocked indefinitely. func (l *CancellableLifetime) TryCheckout() bool { l.mu.RLock() if l.cancelled { @@ -51,7 +54,10 @@ func (l *CancellableLifetime) TryCheckout() bool { return true } -// ReleaseCheckout will decrement the number of current checkouts. +// ReleaseCheckout will decrement the number of current checkouts, it MUST +// only be called after a call to TryCheckout and must not be called more +// than once per call to TryCheckout or else it will panic as it will try +// to unlock an unlocked resource. func (l *CancellableLifetime) ReleaseCheckout() { l.mu.RUnlock() } From a8f8a2dd07aa1d17a29aafcf26fec5eb556cd58d Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Thu, 21 Mar 2019 17:01:06 -0400 Subject: [PATCH 12/12] Add comment to block query method --- src/dbnode/storage/index/block.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/dbnode/storage/index/block.go b/src/dbnode/storage/index/block.go index ccda78bc8b..17b67c7793 100644 --- a/src/dbnode/storage/index/block.go +++ b/src/dbnode/storage/index/block.go @@ -762,7 +762,13 @@ func (b *block) executorWithRLock() (search.Executor, error) { return executor.NewExecutor(readers), nil } -// Query guarentees +// Query acquires a read lock on the block so that the segments +// are guaranteed to not be freed/released while accumulating results. +// This allows references to the mmap'd segment data to be accumulated +// and then copied into the results before this method returns (it is not +// safe to return docs directly from the segments from this method, the +// results datastructure is used to copy it every time documents are added +// to the results datastructure). func (b *block) Query( cancellable *resource.CancellableLifetime, query Query,