Skip to content

Log if artifact downloads are slow#5208

Merged
illicitonion merged 2 commits into
pantsbuild:masterfrom
twitter:dwagnerhall/artifact_cache/slowlog
Dec 15, 2017
Merged

Log if artifact downloads are slow#5208
illicitonion merged 2 commits into
pantsbuild:masterfrom
twitter:dwagnerhall/artifact_cache/slowlog

Conversation

@illicitonion

Copy link
Copy Markdown
Contributor

Right now, sometimes pants looks like it may be hanging or deadlocking
if cache fetches are really slow.

This adds an INFO level log if any particular artifact has spent 60
seconds waiting for 4MB of output from the cache on any particular
thread.

This corresponds with a download speed of less than 67K/s sustained over
at at least 4MB of output artifact.

It's possible that these messages will be displayed more often than once
every 60 seconds, if multiple threads are seeing this slowness in
parallel.

Right now, sometimes pants looks like it may be hanging or deadlocking
if cache fetches are really slow.

This adds an INFO level log if any particular artifact has spent 60
seconds waiting for 4MB of output from the cache on any particular
thread.

This corresponds with a download speed of less than 67K/s sustained over
at at least 4MB of output artifact.

It's possible that these messages will be displayed more often than once
every 60 seconds, if multiple threads are seeing this slowness in
parallel.
@baroquebobcat

Copy link
Copy Markdown
Contributor

Looks reasonable to me. How did you test it?

@illicitonion

Copy link
Copy Markdown
Contributor Author

I set the 60 seconds at 0.1 seconds and saw lots of output lines. I then set it to 1 second and saw a few.

@baroquebobcat baroquebobcat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm modulo the one lint issue!

Could you think of a way to add a unit test for this? I think it's a well-contained change, and I'm not going to block on it. But, I think it's a good idea to have tests for user visible behavior.

@illicitonion

Copy link
Copy Markdown
Contributor Author

Yeah, I gave it some thought originally, but decided that any test would end up being very large and convoluted that it probably wouldn't be worth its value...

@illicitonion illicitonion merged commit ea2c48a into pantsbuild:master Dec 15, 2017
@illicitonion illicitonion deleted the dwagnerhall/artifact_cache/slowlog branch February 26, 2018 11: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.

2 participants