Tsdb/inverted index wiring #6252
Conversation
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. - ingester -0.1%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0.3% |
| i.Delete(labels, fp) | ||
| } | ||
| } | ||
|
|
|
|
||
| func (m *Multi) Add(labels []logproto.LabelAdapter, fp model.Fingerprint) (result labels.Labels) { | ||
| for _, i := range m.indices { | ||
| if i != nil { |
There was a problem hiding this comment.
for curiosity, why would an index inside m.indices be nil?
There was a problem hiding this comment.
In the case where there's only tsdb period configs or non-tsdb period configs (such as boltdb-shipper), only one of the sub-indices will be needed. If that's the case, we can avoid the overhead of running an unnecessary one.
| return m.indexFor(t).LabelValues(name, shard) | ||
| } | ||
|
|
||
| // Query planning is responsible for ensuring no query spans more than one inverted index. |
There was a problem hiding this comment.
is this comment out of place?
There was a problem hiding this comment.
I intended to put it here because most queries in loki use a from, through, but in this case we're using another part of the code base to ensure we'd never query more than one period at a time, so we don't need to account for the range from->through, only the first point (from).
pkg/ingester/index/multi.go
Outdated
| if bitPrefixed == nil { | ||
| bitPrefixed, err = NewBitPrefixWithShards(indexShards) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
nit: maybe you could wrap this error with errors.Wrap and add more details to the message, like which period erroed and its index type. Motivation:
there's a chain of calls that aren't wrapping errors that will end up here, so it might be hard to identify the root of the problem. The chain is:
GetOrCreateInstanceis invoked at multiple places without wrapping errors and triggersnewInstancenewInstancewill invokeindex.NewMultiInvertedIndexwithout wrapping errors- we end up here, with
NewMultiInvertedIndexinvokingNewBitPrefixWithShards
- we end up here, with
| var ( | ||
| err error | ||
|
|
||
| ii Interface // always stored in 0th index |
There was a problem hiding this comment.
does ii stand for invertedIndex? if so I might opt for the longer version of that variable as it's not that long.
|
|
||
| type periodIndex struct { | ||
| time.Time | ||
| idx int // address of the index to use |
There was a problem hiding this comment.
a bit confused about what this represents. it seems to be a type more than an address to me, ie 0 is a regular inverted index, whereas 1 is a tsdb inverted index. could we reflect that in the name and/or comment?
There was a problem hiding this comment.
or is this the index in the indices property on Multi? If so, and there can only be 2, I'm curious if instead of storing these in a fixed length slice with magic indices, maybe Multi should have a field for each type?
There was a problem hiding this comment.
That's a fair point. I wrote it like this in case we had period configs like [boltdb, tsdb, boltdb, tsdb], in which case we'd want the first period to point to the index of the boltdb inverted index, the second to point to the tsdb inverted index and so on. Decoupling the period from the index it points to allows us to instantiate a maximum of one inverted index for each type, regardless of the number of period configs.
The way it's written now will allow us to add other index types in the future if necessary. Does that make sense?
There was a problem hiding this comment.
it does, but we could also come back and update this struct with the necessary types when we get to that point. just feels like we're overloading slice indexing as a mechanism for expressing type, whereas there are clearer ways to do that.
that being said I have no interest in dying on this hill, and I'm cool with keeping it this way if you think it's more flexible.
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. - ingester -0.2%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0.4% |
Followup to #6233
This allows us to run two incompatible inverted indices in the ingesters, one for bit-prefixed sharding (tsdb) and one for modulo sharding (everything else). It also adds some more config validation and fixes a bug in the config-wrapper that hasn't surfaced yet, but will as soon as you try to run a non-current boltdb-shipper period config: d9f1e06
ref #5428