Skip to content

fix: do not rename files on mmap failure#23396

Merged
davidby-influx merged 4 commits intomaster-1.xfrom
DSB_mmap_no_rename
Jun 7, 2022
Merged

fix: do not rename files on mmap failure#23396
davidby-influx merged 4 commits intomaster-1.xfrom
DSB_mmap_no_rename

Conversation

@davidby-influx
Copy link
Copy Markdown
Contributor

If NewTSMReader() fails because mmap fails, do not
rename the file, because the error is probably
caused by vm.max_map_count being too low

closes #23172

If NewTSMReader() fails because mmap fails, do not
rename the file, because the error is probably
caused by vm.max_map_count being too low

closes #23172
@lesam
Copy link
Copy Markdown
Contributor

lesam commented Jun 2, 2022

Looks like a good approach. Can we add a test for it somehow? Maybe dependency-inject a poisoned mmap implementation?

if e := os.Rename(file.Name(), file.Name()+"."+BadTSMFileExtension); e != nil {
f.logger.Error("Cannot rename corrupt tsm file", zap.String("path", file.Name()), zap.Int("id", idx), zap.Error(e))
readerC <- &res{r: df, err: fmt.Errorf("cannot rename corrupt file %s: %v", file.Name(), e)}
if e, ok := err.(MmapError); ok {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be using errors.Is() here instead of a type assertion?

@davidby-influx davidby-influx requested a review from gwossum June 7, 2022 02:48
@davidby-influx davidby-influx marked this pull request as ready for review June 7, 2022 02:48
@davidby-influx davidby-influx merged commit ec412f7 into master-1.x Jun 7, 2022
@davidby-influx davidby-influx deleted the DSB_mmap_no_rename branch June 7, 2022 15:37
davidby-influx added a commit that referenced this pull request Jun 9, 2022
If NewTSMReader() fails because mmap fails, do not
rename the file, because the error is probably
caused by vm.max_map_count being too low

closes #23172

(cherry picked from commit ec412f7)

closes #23413
davidby-influx added a commit that referenced this pull request Jun 10, 2022
If NewTSMReader() fails because mmap fails, do not
rename the file, because the error is probably
caused by vm.max_map_count being too low

closes #23172

(cherry picked from commit ec412f7)

closes #23413
chengshiwen pushed a commit to chengshiwen/influxdb that referenced this pull request Aug 27, 2024
If NewTSMReader() fails because mmap fails, do not
rename the file, because the error is probably
caused by vm.max_map_count being too low

closes influxdata#23172
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants