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

vendor: bump Pebble to e567fec84c6e #81389

Merged

Conversation

jbowens
Copy link
Collaborator

@jbowens jbowens commented May 17, 2022

This commit includes non-trivial changes to account for the change
in the vfs.WithDiskHealthChecks function signature change.
Additionally, this PR uncovered #81413 and necessitated some changes to
work around it.

e567fec8 db: ensure Open closes opened directories on error
b8c9a560 internal/metamorphic: overwrite unused bounds buffers
7c5f0cbb db: copy user-provided bounds and optimize unchanged bounds
f8897076 *: Add IterOption to optionally read L6 filter blocks.
d79f9617 vfs: extend disk-health checking to filesystem metadata operations
5ae21746 db: remove newRangeKeyIter closure
6d975489 db: add BenchmarkIteratorScan
782d102e sstable: fix invariant check for sstable size estimation
738a7f0b db: fix NewIter regression resulting in extra memtable levels
37558663 *: Use keyspan.LevelIter for rangedels in compactions
e6c60c71 db: use sublevel level iters for all compactions out of L0
d8f63bef db: extend documentation on ingested file overlap; improve test cases
b9e970a8 internal/keyspan: correct and document MergingIter key stability
498177bb internal/keyspan: collapse fragmentBoundIterator into MergingIter

Release note (bug fix): Fix a gap in disk-stall detection. Previously,
disk stalls during filesystem metadata operations could go undetected,
inducing deadlocks. Now stalls during these types of operations will
correctly fatal the process.

@jbowens jbowens requested a review from a team as a code owner May 17, 2022 17:46
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jbowens jbowens force-pushed the jackson/pebble-master-e567fec84c6e branch from 07cfcd6 to 685e9ce Compare May 17, 2022 17:56
@jbowens jbowens requested a review from a team as a code owner May 17, 2022 17:56
Copy link
Collaborator

@nicktrav nicktrav left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 9 of 9 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jbowens)

Copy link
Collaborator Author

@jbowens jbowens left a comment

Choose a reason for hiding this comment

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

Still wrangling with two issues:

  1. kv/kvserver.TestStoreMetrics is showing reduced bloom filter lookups.
  2. sql/colexec.TestExternalDistinct has a goroutine leak from the disk-health checking. It appears to be file-level goroutines (not the new filesystem-wide goroutine) that are leaking, but it's a new leak with this commit. Still digging.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jbowens)

@jbowens jbowens force-pushed the jackson/pebble-master-e567fec84c6e branch from 685e9ce to e38c096 Compare May 17, 2022 23:09
@jbowens jbowens requested a review from a team as a code owner May 17, 2022 23:09
@jbowens jbowens force-pushed the jackson/pebble-master-e567fec84c6e branch from e38c096 to 047685b Compare May 17, 2022 23:20
Copy link
Collaborator

@nicktrav nicktrav left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jbowens)

@jbowens jbowens force-pushed the jackson/pebble-master-e567fec84c6e branch from 047685b to 9743844 Compare May 17, 2022 23:48
Copy link
Collaborator

@nicktrav nicktrav left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jbowens)

@yuzefovich
Copy link
Member

Thanks for filing #81413 - I think I have a fix in #81419, do you mind rebasing and checking whether it addresses the issue?

@jbowens
Copy link
Collaborator Author

jbowens commented May 18, 2022

Thanks for filing #81413 - I think I have a fix in #81419, do you mind rebasing and checking whether it addresses the issue?

That fixes it, thanks for the quick turnaround!

@jbowens jbowens force-pushed the jackson/pebble-master-e567fec84c6e branch from 9743844 to 804ed95 Compare May 18, 2022 15:27
@jbowens jbowens requested a review from a team as a code owner May 18, 2022 15:27
@jbowens jbowens force-pushed the jackson/pebble-master-e567fec84c6e branch from 804ed95 to 77acc3f Compare May 18, 2022 15:57
@jbowens
Copy link
Collaborator Author

jbowens commented May 18, 2022

TestLogic is pointing to another open file leak.

[TestLogic] gotest://github.com/cockroachdb/cockroach/pkg/sql/logictest#TestLogic
[12:51:58]
[TestLogic] [Test Output]
=== RUN   TestLogic
    test_log_scope.go:79: test logs captured to: /go/src/github.com/cockroachdb/cockroach/artifacts/logTestLogic1197699296
    test_log_scope.go:80: use -show-logs to present logs inline
=== CONT  TestLogic
    logic.go:4414: -- test log scope end --
    logic_test.go:33: Leaked goroutine: goroutine 11937409 [select]:
        github.com/cockroachdb/pebble/vfs.(*diskHealthCheckingFile).startTicker.func1()
        	/go/src/github.com/cockroachdb/cockroach/vendor/github.com/cockroachdb/pebble/vfs/disk_health.go:72 +0xe8
        created by github.com/cockroachdb/pebble/vfs.(*diskHealthCheckingFile).startTicker
        	/go/src/github.com/cockroachdb/cockroach/vendor/github.com/cockroachdb/pebble/vfs/disk_health.go:67 +0x65
        Leaked goroutine: goroutine 11937584 [select]:
        github.com/cockroachdb/pebble/vfs.(*diskHealthCheckingFile).startTicker.func1()
        	/go/src/github.com/cockroachdb/cockroach/vendor/github.com/cockroachdb/pebble/vfs/disk_health.go:72 +0xe8
        created by github.com/cockroachdb/pebble/vfs.(*diskHealthCheckingFile).startTicker
        	/go/src/github.com/cockroachdb/cockroach/vendor/github.com/cockroachdb/pebble/vfs/disk_health.go:67 +0x65
--- FAIL: TestLogic (1937.22s)

@jbowens jbowens force-pushed the jackson/pebble-master-e567fec84c6e branch from 77acc3f to 52d1182 Compare May 18, 2022 20:38
@jbowens jbowens requested a review from a team as a code owner May 18, 2022 20:38
@jbowens jbowens force-pushed the jackson/pebble-master-e567fec84c6e branch from 52d1182 to ad606d0 Compare May 18, 2022 20:41
Copy link
Collaborator

@nicktrav nicktrav left a comment

Choose a reason for hiding this comment

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

Thanks for persevering with this yak shave! :lgtm:

Reviewed 9 of 13 files at r7, 10 of 10 files at r8, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jbowens)

@yuzefovich
Copy link
Member

I think I found the other problem - #81490 and will open a fix shortly.

@jbowens jbowens force-pushed the jackson/pebble-master-e567fec84c6e branch 2 times, most recently from 0f23bec to aec0ab0 Compare May 19, 2022 15:26
Copy link
Collaborator

@nicktrav nicktrav left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r9, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jbowens)

@jbowens jbowens force-pushed the jackson/pebble-master-e567fec84c6e branch from aec0ab0 to 76b37d0 Compare May 19, 2022 16:39
@jbowens
Copy link
Collaborator Author

jbowens commented May 19, 2022

Hrm, recent pushes aren't tickling CI

@jbowens jbowens force-pushed the jackson/pebble-master-e567fec84c6e branch from 76b37d0 to 1c696b8 Compare May 19, 2022 19:04
@jbowens jbowens requested a review from a team as a code owner May 19, 2022 19:04
@jbowens jbowens requested review from stevendanna and removed request for a team May 19, 2022 19:04
@nicktrav
Copy link
Collaborator

Isn't the lint commit already on master?

@jbowens jbowens force-pushed the jackson/pebble-master-e567fec84c6e branch 3 times, most recently from c62e455 to 31fa14c Compare May 19, 2022 20:39
@jbowens
Copy link
Collaborator Author

jbowens commented May 19, 2022

Yes, it was a bad rebase for poking CI. This doesn't need any review until there's a green CI.

This commit includes non-trivial changes to account for the change
in the `vfs.WithDiskHealthChecks` function signature change.

```
e567fec8 db: ensure Open closes opened directories on error
b8c9a560 internal/metamorphic: overwrite unused bounds buffers
7c5f0cbb db: copy user-provided bounds and optimize unchanged bounds
f8897076 *: Add IterOption to optionally read L6 filter blocks.
d79f9617 vfs: extend disk-health checking to filesystem metadata operations
5ae21746 db: remove newRangeKeyIter closure
6d975489 db: add BenchmarkIteratorScan
782d102e sstable: fix invariant check for sstable size estimation
738a7f0b db: fix NewIter regression resulting in extra memtable levels
37558663 *: Use keyspan.LevelIter for rangedels in compactions
e6c60c71 db: use sublevel level iters for all compactions out of L0
d8f63bef db: extend documentation on ingested file overlap; improve test cases
b9e970a8 internal/keyspan: correct and document MergingIter key stability
498177bb internal/keyspan: collapse fragmentBoundIterator into MergingIter
```

Release note (bug fix): Fix a gap in disk-stall detection. Previously,
disk stalls during filesystem metadata operations could go undetected,
inducing deadlocks.  Now stalls during these types of operations will
correctly fatal the process.
@jbowens jbowens force-pushed the jackson/pebble-master-e567fec84c6e branch from 31fa14c to 1cc2596 Compare May 19, 2022 22:56
Copy link
Collaborator Author

@jbowens jbowens left a comment

Choose a reason for hiding this comment

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

We're green, so I'll merge this afternoon.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nicktrav and @stevendanna)

@jbowens
Copy link
Collaborator Author

jbowens commented May 20, 2022

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented May 20, 2022

Build succeeded:

@craig craig bot merged commit 85d99af into cockroachdb:master May 20, 2022
@jbowens jbowens deleted the jackson/pebble-master-e567fec84c6e branch May 20, 2022 18:26
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.

4 participants