don't bail for missing meta.js files #283
Conversation
krasi-georgiev
changed the title
|
@fabxc is this what you had i mind? |
|
🎬 @fabxc , @gouthamve just need a quick look and will do the tests. |
| @@ -313,13 +314,15 @@ func (c *LeveledCompactor) Compact(dest string, dirs ...string) (uid ulid.ULID, | |||
| for _, d := range dirs { | |||
| b, err := OpenBlock(d, c.chunkPool) | |||
| if err != nil { | |||
| return uid, err | |||
| level.Error(c.logger).Log("msg", "couldn't open a block", "dir", d, "err", err.Error()) | |||
| continue | |||
There was a problem hiding this comment.
Did you check how deletion of blocks after compaction is handled? For example, if the caller just removes all input dirs if we return with no error, that would be kinda bad.
There was a problem hiding this comment.
yes I mentioned in the PR description that wasn't sure what should we do here.
Is a folder without a meta file unusable so just delete with some warning log?
I haven't looked at the code for the retention yet, I guess should do the same there, delete with some warning?
krasi-georgiev
force-pushed
the
meta-file-no-crash-when-missing
branch
from
66a4898
to
0ce9de1
Compare
krasi-georgiev
force-pushed
the
meta-file-no-crash-when-missing
branch
from
0ce9de1
to
639d80a
Compare
krasi-georgiev
force-pushed
the
meta-file-no-crash-when-missing
branch
2 times, most recently
from
2bf06bf
to
353cd6e
Compare
krasi-georgiev
force-pushed
the
meta-file-no-crash-when-missing
branch
3 times, most recently
from
92061c8
to
2c0d5d8
Compare
krasi-georgiev
force-pushed
the
meta-file-no-crash-when-missing
branch
from
2c0d5d8
to
299b2ea
Compare
|
@fabxc PTAL |
|
ping @gouthamve |
|
Hmm this looks good to me but I'd be very cautious about adding this. Like a missing With this, we'll get missing data-ranges and a lot of user-queries as to why the storage is dropping data. I still think error-ing out makes sense rather than deleting data. We could add a command to tsdbtool/promtool that does this cleanup. @krasi-georgiev If this is being accepted, could you add a metric for the number of blocks deleted due to corruption? |
|
I think @fabxc mentioned that this was the behaviour at some point without any bad consequences. @gouthamve if we decide to go this road will add the metrics to the same PR. |
|
closing in favour of #320 best to have this as a separate interface and be run explicitly in case of a data corruption. |
this change ensures that the db can be opened even when one or more folders are missing the meta.js file.
These are skipped when opening the db and deleted when db.reload is called.