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

Fix incorrect usage of binary.Write() in TestUpload(). #587

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

ifoox
Copy link
Collaborator

@ifoox ifoox commented Jul 25, 2024

The call to binary.Write() was previously failing because it was being passed a variable length encoded type (int) rather than a fixed length encoding type. This caused the 2000 blobs tests to actually have 0 blobs. Also fix concurrency expectation for the 2000 blobs test.

Tested:
Ran unit tests.

The call to `binary.Write()` was previously failing because it was being passed a variable length encoded type (int) rather than a fixed length encoding type. This caused the 2000 blobs tests to actually have 0 blobs.
Also fix concurrency expectation for the 2000 blobs test.

Tested:
  Ran unit tests.
@ifoox ifoox requested review from gkousik and mrahs July 25, 2024 02:42
@ifoox ifoox self-assigned this Jul 25, 2024
@mrahs
Copy link
Collaborator

mrahs commented Jul 25, 2024

Curious how you noticed that the blobs were 0 (assuming you mean twoThousandBlobs was empty)? The test was passing before; should it also be updated to capture this issue?

@ifoox
Copy link
Collaborator Author

ifoox commented Jul 25, 2024

I was looking at the test to trace how uploads work (as part of looking at fixing compression stats, internal bug b/312446641), so I added some printfs to the test and noticed something weird happening with 2000 blobs tests, and started debugging from there.

What ended up happening is that twoThousandBlobs was a slice of 2k empty slices ([[],[],[]..., 2k times]), and thousandBlobs was a slice of 1k empty slices.

I'm not sure the test needs to be updated, since it just happened that the test setup was incorrect relative to the original intention but still a valid way test. What the test was testing was:

  • try to do an upload of 2k empty blobs, with the empty blob already existing in the CAS

This is a valid test, it's just not what the original intention of the test was.

@mrahs
Copy link
Collaborator

mrahs commented Jul 25, 2024

I see. Good catch!

On a second look, you already added error checking which should effectively capture the failure prior to your fix.

@ifoox ifoox merged commit a86029c into bazelbuild:master Jul 25, 2024
7 checks passed
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