fix(tsi1/partition/test): fix data races in test code (#57) by devanbenz · Pull Request #25338 · influxdata/influxdb

devanbenz · 2024-09-16T17:39:29Z

fix(tsi1/partition/test): fix data races in test code

This PR is like #24613 but solves it with a setter method for MaxLogFileSize which allows unexporting that value and MaxLogFileAge. There are actually two places locks were needed in test code. The behavior of production code is unchanged.

(cherry picked from commit f0235c4daf4b97769db932f7346c1d3aecf57f8f)

Closes #

Describe your proposed changes here.

I've read the contributing section of the project README.
Signed CLA (if not already signed).

* fix(tsi1/partition/test): fix data races in test code This PR is like #24613 but solves it with a setter method for MaxLogFileSize which allows unexporting that value and MaxLogFileAge. There are actually two places locks were needed in test code. The behavior of production code is unchanged. (cherry picked from commit f0235c4daf4b97769db932f7346c1d3aecf57f8f)

davidby-influx

A few suggestions. Perhaps put them into another PR if you want to cherry-pick them to other branches.

davidby-influx · 2024-09-16T21:29:07Z

tsdb/index/tsi1/partition.go

+func (p *Partition) SetMaxLogFileSize(new int64) (old int64) {
+	p.mu.Lock()
+	old = p.maxLogFileSize
+	p.maxLogFileSize = new


You can do this more idiomatically with

old, p.maxLogFileSize = p.maxLogFileSize, new

👍 Sounds good

davidby-influx · 2024-09-16T21:29:54Z

tsdb/index/tsi1/partition.go

+	defer p.mu.Unlock()

-	var err error
+	if p.fileSet == nil {


davidby-influx · 2024-09-16T21:32:03Z

tsdb/index/tsi1/partition.go

+	var err error
 	for _, f := range p.fileSet.files {
 		if localErr := f.Close(); localErr != nil {
 			err = localErr


Use a []error and errors.Join() here to return all the errors.

https://pkg.go.dev/errors#Join

Oh thats a really cool error handling feature. Will do 👍

closes #24042

davidby-influx

Small change to the use of errrors.Join

davidby-influx · 2024-09-16T22:31:33Z

tsdb/index/tsi1/partition.go

 	for _, f := range p.fileSet.files {
 		if localErr := f.Close(); localErr != nil {
-			err = localErr
+			err = errors.Join(err, localErr)


errors.Join ignores nil errors, so no test for not nil is necessary. The code as it is written will make nested errors; the best thing to do is use a slice.

See here for an example

davidby-influx

LGTM

* fix(tsi1/partition/test): fix data races in test code (#57) * fix(tsi1/partition/test): fix data races in test code This PR is like #24613 but solves it with a setter method for MaxLogFileSize which allows unexporting that value and MaxLogFileAge. There are actually two places locks were needed in test code. The behavior of production code is unchanged. (cherry picked from commit f0235c4daf4b97769db932f7346c1d3aecf57f8f) * feat: modify error handling to be more idiomatic closes #24042 * fix: errors.Join() filters nil errors closes #25341 --------- Co-authored-by: Phil Bracikowski <13472206+philjb@users.noreply.github.com> (cherry picked from commit 5c9e45f)

* fix(tsi1/partition/test): fix data races in test code (#57) * fix(tsi1/partition/test): fix data races in test code This PR is like #24613 but solves it with a setter method for MaxLogFileSize which allows unexporting that value and MaxLogFileAge. There are actually two places locks were needed in test code. The behavior of production code is unchanged. (cherry picked from commit f0235c4daf4b97769db932f7346c1d3aecf57f8f) * feat: modify error handling to be more idiomatic closes #24042 * fix: errors.Join() filters nil errors --------- Co-authored-by: Phil Bracikowski <13472206+philjb@users.noreply.github.com> (cherry picked from commit 5c9e45f)

…25345) * feat: add hook for optimizing series reads based on authorizer (#25207) * fix(tsi1/partition/test): fix data races in test code (#57) (#25338) * fix(tsi1/partition/test): fix data races in test code (#57) * fix(tsi1/partition/test): fix data races in test code This PR is like #24613 but solves it with a setter method for MaxLogFileSize which allows unexporting that value and MaxLogFileAge. There are actually two places locks were needed in test code. The behavior of production code is unchanged. (cherry picked from commit f0235c4daf4b97769db932f7346c1d3aecf57f8f) * feat: modify error handling to be more idiomatic closes #24042 * fix: errors.Join() filters nil errors --------- Co-authored-by: Phil Bracikowski <13472206+philjb@users.noreply.github.com> (cherry picked from commit 5c9e45f) --------- Co-authored-by: Geoffrey Wossum <gwossum@influxdata.com>

…25344) * fix(tsi1/partition/test): fix data races in test code (#57) * fix(tsi1/partition/test): fix data races in test code This PR is like #24613 but solves it with a setter method for MaxLogFileSize which allows unexporting that value and MaxLogFileAge. There are actually two places locks were needed in test code. The behavior of production code is unchanged. (cherry picked from commit f0235c4daf4b97769db932f7346c1d3aecf57f8f) * feat: modify error handling to be more idiomatic closes #24042 * fix: errors.Join() filters nil errors closes #25341 --------- Co-authored-by: Phil Bracikowski <13472206+philjb@users.noreply.github.com> (cherry picked from commit 5c9e45f)

…25344) * fix(tsi1/partition/test): fix data races in test code (#57) * fix(tsi1/partition/test): fix data races in test code This PR is like #24613 but solves it with a setter method for MaxLogFileSize which allows unexporting that value and MaxLogFileAge. There are actually two places locks were needed in test code. The behavior of production code is unchanged. (cherry picked from commit f0235c4daf4b97769db932f7346c1d3aecf57f8f) * feat: modify error handling to be more idiomatic closes #24042 * fix: errors.Join() filters nil errors closes #25341 --------- Co-authored-by: Phil Bracikowski <13472206+philjb@users.noreply.github.com> (cherry picked from commit 5c9e45f) (cherry picked from commit b88e74e) closes #25342

* fix(tsi1/partition/test): fix data races in test code (#57) * fix(tsi1/partition/test): fix data races in test code This PR is like #24613 but solves it with a setter method for MaxLogFileSize which allows unexporting that value and MaxLogFileAge. There are actually two places locks were needed in test code. The behavior of production code is unchanged. (cherry picked from commit f0235c4daf4b97769db932f7346c1d3aecf57f8f) * feat: modify error handling to be more idiomatic closes #24042 * fix: errors.Join() filters nil errors closes #25341 --------- Co-authored-by: Phil Bracikowski <13472206+philjb@users.noreply.github.com> (cherry picked from commit 5c9e45f) (cherry picked from commit b88e74e) closes #25342

#25352) * fix(tsi1/partition/test): fix data races in test code (#57) * fix(tsi1/partition/test): fix data races in test code This PR is like #24613 but solves it with a setter method for MaxLogFileSize which allows unexporting that value and MaxLogFileAge. There are actually two places locks were needed in test code. The behavior of production code is unchanged. (cherry picked from commit f0235c4daf4b97769db932f7346c1d3aecf57f8f) * feat: modify error handling to be more idiomatic closes #24042 * fix: errors.Join() filters nil errors closes #25341 --------- Co-authored-by: Phil Bracikowski <13472206+philjb@users.noreply.github.com> (cherry picked from commit 5c9e45f) (cherry picked from commit b88e74e) closes #25342

… (#25438) * feat: add hook for optimizing series reads based on authorizer (#25207) * fix(tsi1/partition/test): fix data races in test code (#57) (#25338) * fix(tsi1/partition/test): fix data races in test code (#57) * fix(tsi1/partition/test): fix data races in test code This PR is like #24613 but solves it with a setter method for MaxLogFileSize which allows unexporting that value and MaxLogFileAge. There are actually two places locks were needed in test code. The behavior of production code is unchanged. (cherry picked from commit f0235c4daf4b97769db932f7346c1d3aecf57f8f) * feat: modify error handling to be more idiomatic closes #24042 * fix: errors.Join() filters nil errors --------- Co-authored-by: Phil Bracikowski <13472206+philjb@users.noreply.github.com> (cherry picked from commit 5c9e45f) * fix(ci): Update test_pkgs_64bit image to non-eol ubuntu image (#25354) * fix(ci): Update test_pkgs_64bit image to non-eol ubuntu image * fix: trying edge img * fix(ci): update ubuntu image in ci Update test_pkgs_64bit image to non-eol ubuntu image Please see: discuss.circleci.com/t/linux-image-deprecations-and-eol-for-2024/50177 closes #25355 (cherry picked from commit fdf0df7) --------- Co-authored-by: Geoffrey Wossum <gwossum@influxdata.com>

devanbenz requested review from davidby-influx and gwossum September 16, 2024 19:07

davidby-influx requested changes Sep 16, 2024

View reviewed changes

devanbenz mentioned this pull request Sep 16, 2024

fix(tsi1/partition/test): fix data races in test code (#57) #25336

Merged

2 tasks

feat: modify error handling to be more idiomatic

05c5af8

closes #24042

This was referenced Sep 16, 2024

data race between test and product code in partition compaction [Port to master-1.x] #24042

Closed

data race between test and product code in partition compaction [Port to 1.11 branch] #24041

Closed

devanbenz requested a review from davidby-influx September 16, 2024 22:15

davidby-influx requested changes Sep 16, 2024

View reviewed changes

fix: errors.Join() filters nil errors

8f8b488

devanbenz requested a review from davidby-influx September 16, 2024 22:45

davidby-influx approved these changes Sep 16, 2024

View reviewed changes

devanbenz merged commit 5c9e45f into master-1.x Sep 17, 2024

devanbenz deleted the db-backport-57-1.x branch September 17, 2024 01:26

hazzik mentioned this pull request Apr 3, 2025

chore(deps): update github.com/prometheus/prometheus to reduce clutter in go.sum #26215

Open

2 tasks

charlesthomas mentioned this pull request Jul 7, 2025

ct master 1.x circle selfhosted #26591

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(tsi1/partition/test): fix data races in test code (#57)#25338

fix(tsi1/partition/test): fix data races in test code (#57)#25338
devanbenz merged 3 commits intomaster-1.xfrom
db-backport-57-1.x

devanbenz commented Sep 16, 2024

Uh oh!

davidby-influx left a comment

Uh oh!

davidby-influx Sep 16, 2024

Uh oh!

devanbenz Sep 16, 2024

Uh oh!

davidby-influx Sep 16, 2024

Uh oh!

davidby-influx Sep 16, 2024

Uh oh!

devanbenz Sep 16, 2024

Uh oh!

davidby-influx left a comment

Uh oh!

davidby-influx Sep 16, 2024

Uh oh!

devanbenz Sep 16, 2024

Uh oh!

davidby-influx left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

devanbenz commented Sep 16, 2024

Uh oh!

davidby-influx left a comment

Choose a reason for hiding this comment

Uh oh!

davidby-influx Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

devanbenz Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

davidby-influx Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

davidby-influx Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

devanbenz Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

davidby-influx left a comment

Choose a reason for hiding this comment

Uh oh!

davidby-influx Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

devanbenz Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

davidby-influx left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants