Fix failing SearchTagValues endpoint after startup #1813
Conversation
stoewer
force-pushed
the
ingester-restart-error
branch
from
84037c7
to
20b95c6
Compare
stoewer
changed the title
stoewer
requested review from
joe-elliott,
annanay25,
mdisibio,
mapno,
kvrhdn and
zalegrala
as code owners
There was a problem hiding this comment.
I'm torn on this. This is a very correct way to fix this problem, but making changes to the backend interfaces for something I view as temporary is tough. In 2.0 we will drop support for search over the old block format ('v2') and all of this code goes away.
In OpenBackendSearchBlock() we could attempt to call ReadRange() for 1 byte on the header. If that returns "does not exist" then we return ErrSearchNotSupported. I know this is far "hackier" then then the proposed solution, but it will be nicely cleaned up when we delete the rest of this code.
tempodb/backend/local/local.go
Outdated
| objects = append(objects, f.Name()) | ||
| objects := make([]string, 0, len(dirEntries)) | ||
| for _, d := range dirEntries { | ||
| objects = append(objects, d.Name()) |
There was a problem hiding this comment.
This was changed to support the Has() method? I'm concerned about the impact on other uses of List()
There was a problem hiding this comment.
Yes. From the signature alone it is not clear whether List is supposed to return only directory names. The code where List is used (raw.go#L158 or raw.go#L139) seems to anticipate that filenames are listed too. However, the s3 backend also only returns directories / prefixes.
tempodb/backend/raw.go
Outdated
| @@ -113,6 +113,19 @@ func NewReader(r RawReader) Reader { | |||
| } | |||
| } | |||
|
|
|||
| func (r *reader) Has(ctx context.Context, name string, blockID uuid.UUID, tenantID string) (bool, error) { | |||
| objects, err := r.r.List(ctx, KeyPathForBlock(blockID, tenantID)) | |||
There was a problem hiding this comment.
No idea if this works for all backends :). We only really need it to work for local, but that's not ideal.
I also lean towards not changing the backend interface. Tempo is run on a many backends, both the popular clouds but also api-compatible vendor solutions that we have no way to test. Traditionally we have kept Tempo's usage as basic as possible to improve compatibility. I don't necessarily see an issue with adding |
The List method of the local backend now returns both directory and regular file names instead of returning only directroy names
This resolves a bug in SearchTagValues where the method TagValues was called on blocks without a search-header and search-index file
Implement isSearchSupported using ReadRange instead of Has Remove Has method from backend and revert changes in List method
stoewer
force-pushed
the
ingester-restart-error
branch
from
d7aa3f9
to
f95f4d4
Compare
|
Thanks for the review. I removed |
What this PR does:
The bug described in #1792 happened because
rediscoverLocalBlockscreatedBackendSearchBlockobjects for WAL blocks, even when they didn't contain the files required for flatbuffer search (search-headeretc.). Later calls ofTagValues()on the search blocks resulted in the logged error message and caused the/tempopb.Querier/SearchTagValuesendpoint to fail.This PR skips the creation of
BackendSearchBlockfor WAL blocks that do not contain the flatbuffer search files.Which issue(s) this PR fixes:
Fixes #1792
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]