Skip to content

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

Merged
devanbenz merged 1 commit intomain-2.xfrom
db-backport-57
Sep 16, 2024
Merged

fix(tsi1/partition/test): fix data races in test code (#57)#25336
devanbenz merged 1 commit intomain-2.xfrom
db-backport-57

Conversation

@devanbenz
Copy link
Copy Markdown

  • 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)
Copy link
Copy Markdown
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.

LGTM

@devanbenz devanbenz merged commit 5a59938 into main-2.x Sep 16, 2024
@devanbenz devanbenz deleted the db-backport-57 branch September 16, 2024 21:51

// Wait will block until all compactions are finished.
// Must only be called while they are disabled.
// must only be called while they are disabled.
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.

I think some search and replace for mu de-capitalized this must.

Copy link
Copy Markdown
Author

@devanbenz devanbenz Sep 16, 2024

Choose a reason for hiding this comment

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

@gwossum --
@davidby-influx suggested some changes in #25338 so I'll make those changes and do must -> Must and cherry pick in to all branches.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants