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

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


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


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

tsdb/index/tsi1/partition.go


 	// Close log files.
+	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

@@ -389,7 +388,7 @@ func (p *Partition) Close() error {
 	var err error
 	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 <[email protected]> (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 <[email protected]> (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 <[email protected]> (cherry picked from commit 5c9e45f) --------- Co-authored-by: Geoffrey Wossum <[email protected]>

…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 <[email protected]> (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 <[email protected]> (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 <[email protected]> (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 <[email protected]> (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 <[email protected]> (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 <[email protected]>

devanbenz requested review from gwossum and davidby-influx 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
9 checks passed

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

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 commented Sep 16, 2024

davidby-influx left a comment

davidby-influx Sep 16, 2024

devanbenz Sep 16, 2024

davidby-influx Sep 16, 2024

davidby-influx Sep 16, 2024

devanbenz Sep 16, 2024

davidby-influx left a comment

davidby-influx Sep 16, 2024

devanbenz Sep 16, 2024

davidby-influx left a comment

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

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

Conversation

devanbenz commented Sep 16, 2024

davidby-influx left a comment

Choose a reason for hiding this comment

davidby-influx Sep 16, 2024

Choose a reason for hiding this comment

devanbenz Sep 16, 2024

Choose a reason for hiding this comment

davidby-influx Sep 16, 2024

Choose a reason for hiding this comment

davidby-influx Sep 16, 2024

Choose a reason for hiding this comment

devanbenz Sep 16, 2024

Choose a reason for hiding this comment

davidby-influx left a comment

Choose a reason for hiding this comment

davidby-influx Sep 16, 2024

Choose a reason for hiding this comment

devanbenz Sep 16, 2024

Choose a reason for hiding this comment

davidby-influx left a comment

Choose a reason for hiding this comment