Merge lp:~julian-edwards/gwacl/fix-block-lengths into lp:gwacl

Proposed by Julian Edwards on 2013-08-05
Status: Merged
Approved by: Julian Edwards on 2013-08-05
Approved revision: 210
Merged at revision: 210
Proposed branch: lp:~julian-edwards/gwacl/fix-block-lengths
Merge into: lp:gwacl
Diff against target: 42 lines (+7/-5)
2 files modified
storage.go (+2/-0)
storage_test.go (+5/-5)
To merge this branch: bzr merge lp:~julian-edwards/gwacl/fix-block-lengths
Reviewer Review Type Date Requested Status
Raphaël Badin (community) 2013-08-05 Approve on 2013-08-05
Review via email: mp+178522@code.launchpad.net

Commit message

Ensure consistent block lengths when uploading large files.

To post a comment you must log in.
Raphaël Badin (rvb) wrote :

Looks good.

[0]

8 + // Block IDs must be a consistent length, so pad it out.
9 + blockID = fmt.Sprintf("%030s", blockID)

I was just wondering about the choice of "30" here but I guess it does not really matter. Maybe worth turning it into a private variable though, especially if you do what I suggest in [1].

[1]

21 - assertBlockSent(c, context, data, b64("0"), transport.Exchanges[0])
22 + assertBlockSent(c, context, data, b64("000000000000000000000000000000"), transport.Exchanges[0])
etc.

Using strings.Repeat would be a bit more elegant here.

review: Approve
Julian Edwards (julian-edwards) wrote :

On Monday 05 Aug 2013 09:41:25 Raphaël Badin wrote:
> Review: Approve
>
> Looks good.

Thanks for reviewing Mr B.

>
> [0]
>
> 8 + // Block IDs must be a consistent length, so pad it out.
> 9 + blockID = fmt.Sprintf("%030s", blockID)
>
> I was just wondering about the choice of "30" here but I guess it does not
> really matter. Maybe worth turning it into a private variable though,
> especially if you do what I suggest in [1].

Entirely random but there's no point making it small. I used the value
suggested in the bug since it's likely to work.

>
> [1]
>
> 21 - assertBlockSent(c, context, data, b64("0"),
transport.Exchanges[0])
> 22 + assertBlockSent(c, context, data,
> b64("000000000000000000000000000000"), transport.Exchanges[0]) etc.
>
> Using strings.Repeat would be a bit more elegant here.

The problem with things like that is that you could modify the private
variable and no test would fail. Now some may consider that good, but
personally I start to feel suspicious and would end up poking around to work
out why. I'd much rather have tests that test outcomes entirely independently
of internal code parameters.

Raphaël Badin (rvb) wrote :

> The problem with things like that is that you could modify the private
> variable and no test would fail. Now some may consider that good, but
> personally I start to feel suspicious and would end up poking around to work
> out why. I'd much rather have tests that test outcomes entirely independently
> of internal code parameters.

I understand your position but I still like the internal parameter solution. Now your tests test two different things: the behavior *and* the size of the implicit internal parameter. If you want to be thorough, I would suggest having 2 tests: one that would test the behavior (and that one would use the internal parameter) and another one that would test the size of the padding.

Now I agree that this might be a bit overkill in this precise situation.

Julian Edwards (julian-edwards) wrote :

On Monday 05 Aug 2013 10:40:36 you wrote:
> I understand your position but I still like the internal parameter solution.
> Now your tests test two different things: the behavior *and* the size of
> the implicit internal parameter.

I really don't see a problem with increased test coverage. ;)

> If you want to be thorough, I would
> suggest having 2 tests: one that would test the behavior (and that one
> would use the internal parameter) and another one that would test the size
> of the padding.
>
> Now I agree that this might be a bit overkill in this precise situation.

Yes, I think we can agree on that!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'storage.go'
2--- storage.go 2013-07-02 02:29:44 +0000
3+++ storage.go 2013-08-05 09:31:23 +0000
4@@ -35,6 +35,8 @@
5 }
6 block := bytes.NewReader(buffer[:blockSize])
7 blockID := strconv.FormatInt(blockNum, 36)
8+ // Block IDs must be a consistent length, so pad it out.
9+ blockID = fmt.Sprintf("%030s", blockID)
10 Debugf("Uploading block %d (size=%d, id=%s).\n",
11 blockNum, blockSize, blockID)
12 err = context.PutBlock(container, filename, blockID, block)
13
14=== modified file 'storage_test.go'
15--- storage_test.go 2013-07-17 09:11:43 +0000
16+++ storage_test.go 2013-08-05 09:31:23 +0000
17@@ -30,9 +30,9 @@
18 // There were two exchanges.
19 c.Assert(transport.ExchangeCount, Equals, 2)
20 // The first request is a Put Block with the block data.
21- assertBlockSent(c, context, data, b64("0"), transport.Exchanges[0])
22+ assertBlockSent(c, context, data, b64("000000000000000000000000000000"), transport.Exchanges[0])
23 // The second request is Put Block List to commit the block above.
24- assertBlockListSent(c, context, []string{b64("0")}, transport.Exchanges[1])
25+ assertBlockListSent(c, context, []string{b64("000000000000000000000000000000")}, transport.Exchanges[1])
26 }
27
28 func (suite *testUploadBlockBlob) TestLargeFile(c *C) {
29@@ -49,10 +49,10 @@
30 c.Assert(transport.ExchangeCount, Equals, 3)
31 // The first two requests are Put Block with chunks of the block data. The
32 // weird looking block IDs are base64 encodings of the strings "0" and "1".
33- assertBlockSent(c, context, data[:1024*1024], b64("0"), transport.Exchanges[0])
34- assertBlockSent(c, context, data[1024*1024:], b64("1"), transport.Exchanges[1])
35+ assertBlockSent(c, context, data[:1024*1024], b64("000000000000000000000000000000"), transport.Exchanges[0])
36+ assertBlockSent(c, context, data[1024*1024:], b64("000000000000000000000000000001"), transport.Exchanges[1])
37 // The second request is Put Block List to commit the block above.
38- assertBlockListSent(c, context, []string{b64("0"), b64("1")}, transport.Exchanges[2])
39+ assertBlockListSent(c, context, []string{b64("000000000000000000000000000000"), b64("000000000000000000000000000001")}, transport.Exchanges[2])
40 }
41
42 func uploadRandomBlob(c *C, context *StorageContext, size int) []byte {

Subscribers

People subscribed via source and target branches

to all changes: