storage: add TestMVCCHistories metamorphic param for TBIs
#86256
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
|
@jbowens In addition to the issue in cockroachdb/pebble#1895, this also occasionally fails with a goroutine leak, after about 6 seconds. Am I holding this wrong somehow? |
erikgrinaker
force-pushed
the
mvcc-histories-metamorphic-tbi
branch
from
5790d4c
to
169c1e4
Compare
tbg
approved these changes
Aug 17, 2022
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @jbowens)
jbowens
approved these changes
Aug 17, 2022
There was a problem hiding this comment.
Filed cockroachdb/pebble#1898 for the goroutine exit race at Close.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:
|
@jbowens Any chance we can get that race fixed? I'd really like to merge this (and get some similar metamorphic params in) for better TBI test coverage. |
erikgrinaker
force-pushed
the
mvcc-histories-metamorphic-tbi
branch
from
169c1e4
to
2ea11a4
Compare
|
@jbowens Afraid I'm still seeing goroutine leaks after the Pebble bump, even when I give it 30 seconds to clean up. This only happens when I call |
jbowens
added a commit
to jbowens/cockroach
that referenced
this pull request
Aug 31, 2022
Fix a couple instances of leaked iterators in unit tests in the storage package. This should also resolve the goroutine leak observed in cockroachdb#86256. Informs cockroachdb#71481. Release justification: Non-production code changes Release note: None
craig bot
pushed a commit
that referenced
this pull request
Aug 31, 2022
86363: bazel: make `go test` process timeout before bazel kills it r=rickystewart a=healthy-pod Previously, bazel will kill the test process if it goes beyond the timeout duration set for its size. This prevented us from knowing which tests timed out and also prevented us from getting their stack traces. This patch causes the `go test` process to timeout before bazel kills it to allow us to know which test timed out and get its stack trace. Closes #78185 Release justification: Non-production code changes Release note: None 87067: descs,catkv: rewrite Collection backend r=postamar a=postamar This rewrite was motivated by the upcoming need to validate descriptors up to different levels depending on why they were being read. Doing so requires tracking the validation level in a more granular fashion within the Collection's caches. Trying to make this work within the existing scheme where the uncommitted and stored descriptors are all in the same layer turned out to be painful and hard to reason about. This commit does several things: 1. The uncommitted descriptors are spun off into their own distinct layer again. 2. The stored descriptors now mirror what's in storage and so that whole layer could effectively be moved to catkv. 3. Execution flow through the layers in the Collection is now straightforwardly implemented and easy to reason about. Validation and hydration now take place in unique, well-defined steps at the very end of a lookup, be it by name or by ID. 3. Instances of this new stored descriptors layer are now also used by the lease manager, the Direct() interface provided by the Collection, etc. 4. As a result the logic in catkv could be significantly cleaned up. In particular, lookups for descriptor validation are significantly less convoluted now. 5. The SystemNamespaceCache object was also moved to catkv and now supports caching descriptors, and so was renamed to SystemCatalogCache. Despite significant rewriting of the inner workings of the descriptors collection, this commit should not functionally change anything. Informs #85263. Release justification: low-risk, high-benefit change. Release note: None 87103: storage: fix mvcc stats on gc of range tombstone over del r=erikgrinaker a=aliher1911 Previously if range tombstone was placed over delete, GC would not correctly update GC bytes age incorrectly using range tombstone ts as age of underlying tombstone. Release justification: Bugfix Release note: None Fixing: #87042 87212: ci: set appropriate test tmpdir for compose test r=srosenberg a=rickystewart Otherwise the tmpdir defaults to `/artifacts` which is non-existent outside of a Docker container. Release justification: Non-production code changes Release note: None 87229: storage: fix leaked iterators in unit tests r=erikgrinaker a=jbowens Fix a couple instances of leaked iterators in unit tests in the storage package. This should also resolve the goroutine leak observed in #86256. Informs #71481. Release justification: Non-production code changes Release note: None Co-authored-by: healthy-pod <[email protected]> Co-authored-by: Marius Posta <[email protected]> Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: Oleg Afanasyev <[email protected]> Co-authored-by: Ricky Stewart <[email protected]> Co-authored-by: Jackson Owens <[email protected]>
This patch adds the metamorphic test parameter `mvcc-histories-separate-engine-blocks`, which will configure Pebble with very small blocks and trigger a flush after every command. This may uncover bugs in TBI optimizations. Release justification: non-production code changes Release note: None
erikgrinaker
force-pushed
the
mvcc-histories-metamorphic-tbi
branch
from
2ea11a4
to
853c531
Compare
|
Thanks for getting to the bottom of the goroutine leaks here @jbowens! CI failed with some weird bors r=tbg,jbowens |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch adds the metamorphic test parameter
mvcc-histories-separate-engine-blocks, which will configure Pebblewith very small blocks and trigger a flush after every command. This
may uncover bugs in TBI optimizations.
Release justification: non-production code changes
Release note: None