-
Notifications
You must be signed in to change notification settings - Fork 65
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
*: update parquet-go to include goroutine leak fix #178
Conversation
79e96cc
to
e5f58fd
Compare
Interesting failure mode on first CI run. Looks like an unrelated flake:
The test only seems to spawn 8 goroutines. Trying to reproduce the failure locally. |
Looks like the cause if a bunch of leaked goroutines in
Looks like we're spawning 100k goroutines in a single test run. I think the likeliest thing is that this is a latent bug in our page lifecycle management that has only surfaced due to segmentio/parquet-go#297. |
The last commit fixes the |
d55d237
to
6fd3374
Compare
Apologies for the excess notifications. I'm going to try peppering the code with some missing |
I found a potential goroutine leak in parquet (fixed in segmentio/parquet-go#337) which I think fixes our problem (at least makes the leak detector not scream at me when running without finalizers). I'll merge the upstream fix, update the version on this PR and then retry. |
This should alleviate our CI flakes where too many goroutines are alive at one time. There is also a fix pulled in for buffering bloom filter reads.
The recent parquet version upgrade stressed a latent bug in our code. Lots of places were missing a rows.Close call, resulting in leaked goroutines.
6fd3374
to
8aac262
Compare
RFAL, I pulled in the upstream goroutine leak fix as well as the bloom filter read buffering @thorfour |
This should alleviate our CI flakes where too many goroutines are alive at one
time.
There is also a fix pulled in for buffering bloom filter reads.