Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not rename files on mmap failure #25340

Merged
merged 1 commit into from
Sep 17, 2024
Merged

fix: do not rename files on mmap failure #25340

merged 1 commit into from
Sep 17, 2024

Conversation

gwossum
Copy link
Member

@gwossum gwossum commented Sep 16, 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: #25337

(cherry picked from commit ec412f7)

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: #25337

(cherry picked from commit ec412f7)
@gwossum gwossum self-assigned this Sep 16, 2024
@gwossum gwossum added area/tsm area/2.x OSS 2.0 related issues and PRs labels Sep 16, 2024
@gwossum gwossum marked this pull request as ready for review September 16, 2024 22:21
Copy link
Contributor

@davidby-influx davidby-influx left a comment

Choose a reason for hiding this comment

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

A few small changes

// be left untouched, and the vm.max_map_count be raised.
f.logger.Error("Cannot read TSM file, system limit for vm.max_map_count may be too low",
zap.String("path", file.Name()), zap.Int("id", idx), zap.Error(err))
readerC <- &res{r: df, err: fmt.Errorf("cannot read file %s, system limit for vm.max_map_count may be too low: %v", file.Name(), err)}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use a %w here instead of %v

Copy link
Member Author

Choose a reason for hiding this comment

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

The master-1.x code uses %v also. My preference would be to get this PR merged and backported to 2.7, then do a PR to fix this in master-1.x and propagate it through the branches.

readerC <- &res{r: df, err: fmt.Errorf("cannot rename corrupt file %s: %v", file.Name(), e)}
return
}
readerC <- &res{r: df, err: fmt.Errorf("cannot read corrupt file %s: %v", file.Name(), err)}
Copy link
Contributor

Choose a reason for hiding this comment

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

%w instead of %v

Copy link
Contributor

@davidby-influx davidby-influx left a comment

Choose a reason for hiding this comment

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

With the %v left for another time.

@gwossum gwossum merged commit 5aff511 into main-2.x Sep 17, 2024
25 checks passed
@gwossum gwossum deleted the gw_25337 branch September 17, 2024 17:48
@gwossum gwossum mentioned this pull request Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tsm area/2.x OSS 2.0 related issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants