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.

Sign up for GitHub

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

Attempt to improve upload performance #3501

Merged
merged 9 commits into from
Aug 2, 2023
+631 −250
Merged

Attempt to improve upload performance #3501

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
77fa134
Pick lint
webbnh Jul 24, 2023
982e992
Use subprocess tar command to extract files from tarball
webbnh Jul 24, 2023
c6ea0d3
Add cleanup for the Popen object in Inventory.close()
webbnh Jul 26, 2023
7f8f75f
Pick lint from cache manager unit tests
webbnh Jul 27, 2023
4d8f81a
Unit test update
webbnh Jul 28, 2023
149af09
Polish unit test changes
webbnh Jul 31, 2023
b22cee5
Don't wait forever for the tar command
webbnh Jul 31, 2023
6507be1
Review feedback
webbnh Jul 31, 2023
2e5aa94
No good deed....
webbnh Aug 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions lib/pbench/test/unit/server/test_cache_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1029,9 +1029,7 @@ def poll(self) -> Optional[int]:
return poll_return

def kill(self) -> None:
raise AssertionError(
"This test doesn't expect the stream to be closed."
)
pass

def mock_shutil_which(
cmd: str, _mode: int = os.F_OK | os.X_OK, _path: Optional[str] = None
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.close that 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... 🙂).

Copy link
Member

Choose a reason for hiding this comment

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

You should not wait for me to check the test code before you merge. I don't know how long it will take me 😁

Copy link
Member

Choose a reason for hiding this comment

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

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)....

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.


try:
got = Tarball.extract(tar, path)
Expand Down