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

[Julian Edwards]

Ensure consistent block lengths when uploading large files.

[Raphaël Badin (rvb)]

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

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

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

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!

=== modified file 'storage.go'
--- storage.go	2013-07-02 02:29:44 +0000
+++ storage.go	2013-08-05 09:31:23 +0000
@@ -35,6 +35,8 @@
         }
         block := bytes.NewReader(buffer[:blockSize])
         blockID := strconv.FormatInt(blockNum, 36)
+        // Block IDs must be a consistent length, so pad it out.
+        blockID = fmt.Sprintf("%030s", blockID)
         Debugf("Uploading block %d (size=%d, id=%s).\n",
             blockNum, blockSize, blockID)
         err = context.PutBlock(container, filename, blockID, block)

=== modified file 'storage_test.go'
--- storage_test.go	2013-07-17 09:11:43 +0000
+++ storage_test.go	2013-08-05 09:31:23 +0000
@@ -30,9 +30,9 @@
     // There were two exchanges.
     c.Assert(transport.ExchangeCount, Equals, 2)
     // The first request is a Put Block with the block data.
-    assertBlockSent(c, context, data, b64("0"), transport.Exchanges[0])
+    assertBlockSent(c, context, data, b64("000000000000000000000000000000"), transport.Exchanges[0])
     // The second request is Put Block List to commit the block above.
-    assertBlockListSent(c, context, []string{b64("0")}, transport.Exchanges[1])
+    assertBlockListSent(c, context, []string{b64("000000000000000000000000000000")}, transport.Exchanges[1])
 }
 
 func (suite *testUploadBlockBlob) TestLargeFile(c *C) {
@@ -49,10 +49,10 @@
     c.Assert(transport.ExchangeCount, Equals, 3)
     // The first two requests are Put Block with chunks of the block data. The
     // weird looking block IDs are base64 encodings of the strings "0" and "1".
-    assertBlockSent(c, context, data[:1024*1024], b64("0"), transport.Exchanges[0])
-    assertBlockSent(c, context, data[1024*1024:], b64("1"), transport.Exchanges[1])
+    assertBlockSent(c, context, data[:1024*1024], b64("000000000000000000000000000000"), transport.Exchanges[0])
+    assertBlockSent(c, context, data[1024*1024:], b64("000000000000000000000000000001"), transport.Exchanges[1])
     // The second request is Put Block List to commit the block above.
-    assertBlockListSent(c, context, []string{b64("0"), b64("1")}, transport.Exchanges[2])
+    assertBlockListSent(c, context, []string{b64("000000000000000000000000000000"), b64("000000000000000000000000000001")}, transport.Exchanges[2])
 }
 
 func uploadRandomBlob(c *C, context *StorageContext, size int) []byte {