Attempt to improve upload performance #3501
Conversation
webbnh
force-pushed
the
extract_first_metadata
branch
from
09b0581
to
f733bdd
Compare
webbnh
force-pushed
the
extract_first_metadata
branch
from
f733bdd
to
de1e124
Compare
webbnh
force-pushed
the
extract_first_metadata
branch
2 times, most recently
from
3c9b2fb
to
806af82
Compare
webbnh
force-pushed
the
extract_first_metadata
branch
3 times, most recently
from
85502f5
to
828d815
Compare
webbnh
force-pushed
the
extract_first_metadata
branch
9 times, most recently
from
940f84c
to
9ba86ed
Compare
webbnh
force-pushed
the
extract_first_metadata
branch
from
9ba86ed
to
b22cee5
Compare
|
This is now ready for review. |
webbnh
force-pushed
the
extract_first_metadata
branch
from
a8d06fc
to
6507be1
Compare
| @@ -1042,6 +1040,7 @@ def mock_shutil_which( | |||
| with monkeypatch.context() as m: | |||
| m.setattr(shutil, "which", mock_shutil_which) | |||
| m.setattr(subprocess, "Popen", MockPopen) | |||
| m.setattr(Inventory, "close", MockBufferedReader.close) | |||
There was a problem hiding this comment.
I suppose that's fine that you're mocking Inventory.close to a different close you also don't expect to be called, and you cover all of the Inventory class below, but it looks odd and the self-documentation here (plus the assert if it is called) could be better. Ah, well.
There was a problem hiding this comment.
It should be fine to mock Inventory.close() with a different close(), given that they are all supposed to be providing the same io.IOBase.close() interface! 😆 (And, yeah, since I expect none of them to be called, using separate implementations seems silly and redundant. 😉)
Nevertheless, I don't disagree that, on the surface, it looks funny. 🙂
As for the documentation...well, Nick is still looking at the test code, so there is still a chance to fix that. 🙃 What in particular would you like to see?
There was a problem hiding this comment.
Again, the clearest documentation would be a mocked Inventory.close that doesn't claim to be a stream close. I recognize that's "wasted" code, but it's not much. Second best would be a comment that we're just being lazy here because it doesn't really matter as we don't expect it to be called. (And if you're at all like me, once you start to write that and think about how it'll look, you'll probably just write the "proper" mock instead.)
There was a problem hiding this comment.
once you start to write that and think about how it'll look, you'll probably just write the "proper" mock instead.
One of the problems that I'm trying to address is to minimize the amount of code in the unit tests which is not executed per the coverage report. So, adding an additional mock which causes the test to fail if it is called actually moves away from that goal.
the clearest documentation would be a mocked
Inventory.closethat doesn't claim to be a stream close.
I find this to be curious, because, AIUI, we want Inventory to act like a stream, right? (I mean, it provides all the same methods with basically the same semantics.) So, it makes sense to me to use a stream-close mock for Inventory.close() (in this weird instance, anyway)....
Second best would be a comment that we're just being lazy here because it doesn't really matter as we don't expect it to be called.
I'll do that, if you think it's worth it (although, I don't think I'll term it as "lazy", because that's not really what it's accomplishing for me... 🙂).
There was a problem hiding this comment.
You should not wait for me to check the test code before you merge. I don't know how long it will take me 😁
There was a problem hiding this comment.
I find this to be curious, because, AIUI, we want
Inventoryto act like a stream, right? (I mean, it provides all the same methods with basically the same semantics.) So, it makes sense to me to use a stream-close mock forInventory.close()(in this weird instance, anyway)....
Inventory composes a stream and a "stream provider" so they can be managed together. That composition is critical, and eliding that to mock it as if it were a stream just seems really obscure and awkward to me. But what you've got will work, even if it might confuse a future reader who even notices it. I approved, so I'll leave the rest to you.
The recent "Ops Review" session revealed that upload requests time out if the file approaches 7Gb in size. Profiling the execution indicates that the bulk of the time is spent extracting the
metadata.logfile from the results tarball. This is due to a "feature" of tar archives: the same file may appear more than once, and so an extraction has to search the entire archive to ensure that the member being extracted is the most up-to-date (i.e., last) one, and this is punishing as the archive size grows. (Also, the Python Standard librarytarfilepackage is not as performant as the standalonetar(1)utility, and this costs additional time.)This PR changes the Server to spawn a subprocess to run the
tar(1)utility, which allows us to specify the--occurrenceoption which allows us to abort the search after the first instance of the member is found. Since Pbench owns the construction of the archive, we know that each member occurs only once, so there is no need to search for other instances of it. Also, since themetadata.logfile occurs near the beginning of the archive, by aborting the search when we find the first instance of the member, we are able to reduce the overall search time from multiple minutes to a fraction of a second.This PR consists of a series of commits. Two of them are the changes to the cache manager module to add the subprocess execution and the changes to the corresponding unit tests. Two of them are passes over the cache manager module and the corresponding tests to "pick lint". The others are iteration on the respective changes to the code and the tests.