redo: don't re-provide blocks we've provided very recently (take 2)

this redoes (again) 582e5de. License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
ipfs · Oct 6, 2016 · aab19fd · whyrusleeping · Oct 8, 2016 · whyrusleeping
1 parent f50bda3
commit aab19fd
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 38 deletions.
diff --git a/blockservice/blockservice.go b/blockservice/blockservice.go
@@ -37,8 +37,9 @@ type BlockService interface {
 
 type blockService struct {
 	// TODO don't expose underlying impl details
-	blockstore blockstore.Blockstore
-	exchange   exchange.Interface
+	blockstore   blockstore.Blockstore
+	exchange     exchange.Interface
+	checkFirst   bool
 }
 
 // an Object is simply a typed block
@@ -56,6 +57,21 @@ func New(bs blockstore.Blockstore, rem exchange.Interface) BlockService {
 	return &blockService{
 		blockstore: bs,
 		exchange:   rem,
+		checkFirst: true,
+	}
+}
+
+// NewWriteThrough ceates a BlockService that guarantees writes we go
+// through to the blockstore
+func NewWriteThrough(bs blockstore.Blockstore, rem exchange.Interface) BlockService {
+	if rem == nil {
+		log.Warning("blockservice running in local (offline) mode.")
+	}
+
+	return &blockService{
+		blockstore: bs,
+		exchange:   rem,
+		checkFirst: false,
 	}
 }
 
@@ -70,22 +86,19 @@ func (bs *blockService) Exchange() exchange.Interface {
 // AddBlock adds a particular block to the service, Putting it into the datastore.
 // TODO pass a context into this if the remote.HasBlock is going to remain here.
 func (s *blockService) AddObject(o Object) (*cid.Cid, error) {
-	// TODO: while this is a great optimization, we should think about the
-	// possibility of streaming writes directly to disk. If we can pass this object
-	// all the way down to the datastore without having to 'buffer' its data,
-	// we could implement a `WriteTo` method on it that could do a streaming write
-	// of the content, saving us (probably) considerable memory.
 	c := o.Cid()
-	has, err := s.blockstore.Has(key.Key(c.Hash()))
-	if err != nil {
-		return nil, err
-	}
+	if s.checkFirst {
+		has, err := s.blockstore.Has(key.Key(c.Hash()))
+		if err != nil {
+			return nil, err
+		}
 
-	if has {
-		return c, nil
+		if has {
+			return c, nil
+		}
 	}
 
-	err = s.blockstore.Put(o)
+	err := s.blockstore.Put(o)
 	if err != nil {
 		return nil, err
 	}
@@ -103,13 +116,14 @@ func (s *blockService) AddObjects(bs []Object) ([]*cid.Cid, error) {
 	for _, b := range bs {
 		c := b.Cid()
 
-		has, err := s.blockstore.Has(key.Key(c.Hash()))
-		if err != nil {
-			return nil, err
-		}
-
-		if has {
-			continue
+		if s.checkFirst {
+			has, err := s.blockstore.Has(key.Key(c.Hash()))
+			if err != nil {
+				return nil, err
+			}
+			if has {
+				continue
+			}
 		}
 
 		toput = append(toput, b)

diff --git a/core/commands/add.go b/core/commands/add.go
@@ -179,7 +179,7 @@ You can now refer to the added file in a gateway, like so:
 				return
 			}
 			blockstore := filestore_support.NewBlockstore(n.Blockstore, fs)
-			blockService := bserv.New(blockstore, exchange)
+			blockService := bserv.NewWriteThrough(blockstore, exchange)
 			dagService := dag.NewDAGService(blockService)
 			fileAdder, err = coreunix.NewAdder(req.Context(), n.Pinning, blockstore, dagService, useRoot)
 			fileAdder.FullName = true
@@ -188,7 +188,7 @@ You can now refer to the added file in a gateway, like so:
 			// add directly to the first mount bypassing
 			// the Has() check of the multi-blockstore
 			blockstore := bs.NewGCBlockstore(n.Blockstore.FirstMount(), n.Blockstore)
-			blockService := bserv.New(blockstore, exchange)
+			blockService := bserv.NewWriteThrough(blockstore, exchange)
 			dagService := dag.NewDAGService(blockService)
 			fileAdder, err = coreunix.NewAdder(req.Context(), n.Pinning, blockstore, dagService, useRoot)
 		} else if exchange != n.Exchange {

diff --git a/test/sharness/lib/test-filestore-lib.sh b/test/sharness/lib/test-filestore-lib.sh
@@ -80,7 +80,7 @@ test_post_add() {
         test_must_fail ipfs cat "$HASH" >/dev/null
     '
 
-    test_expect_failure "okay after re-adding under new name" '
+    test_expect_success "okay after re-adding under new name" '
         ipfs $cmd "$dir"/mountdir/hello2.txt 2> add.output &&
         ipfs cat "$HASH" >/dev/null
     '
@@ -210,15 +210,15 @@ filestore_test_exact_paths() {
       echo "Hello Worlds!" > dirlink/hello.txt
     '
 
-    test_expect_failure "ipfs filestore add $opts adds under the expected path name (with symbolic links)" '
+    test_expect_success "ipfs filestore add $opts adds under the expected path name (with symbolic links)" '
       FILEPATH="`pwd`/dirlink/hello.txt" &&
       ipfs filestore add $opt "$FILEPATH" &&
       echo "$FILEPATH" > ls-expected &&
       ipfs filestore ls-files -q QmVr26fY1tKyspEJBniVhqxQeEjhF78XerGiqWAwraVLQH > ls-actual &&
       test_cmp ls-expected ls-actual
     '
 
-    test_expect_failure "ipfs filestore ls dirlink/ works as expected" '
+    test_expect_success "ipfs filestore ls dirlink/ works as expected" '
       echo "QmVr26fY1tKyspEJBniVhqxQeEjhF78XerGiqWAwraVLQH" > ls-expected
       ipfs filestore ls -q "`pwd`/dirlink/" > ls-actual
       test_cmp ls-expected ls-actual

diff --git a/test/sharness/t0260-filestore.sh b/test/sharness/t0260-filestore.sh
@@ -169,13 +169,13 @@ test_expect_success "testing filestore dups pinned" '
   test_cmp dups-actual dups-expected
 '
 
-test_expect_failure "testing filestore dups unpinned" '
+test_expect_success "testing filestore dups unpinned" '
   ipfs filestore dups unpinned > dups-actual &&
   echo QmPrrHqJzto9m7SyiRzarwkqPcCSsKR2EB1AyqJfe8L8tN > dups-expected &&
   test_cmp dups-actual dups-expected
 '
 
-test_expect_failure "testing filestore dups" '
+test_expect_success "testing filestore dups" '
   ipfs filestore dups > dups-out &&
   grep QmZm53sWMaAQ59x56tFox8X9exJFELWC33NLjK6m8H7CpN dups-out &&
   grep QmPrrHqJzto9m7SyiRzarwkqPcCSsKR2EB1AyqJfe8L8tN dups-out

diff --git a/test/sharness/t0265-filestore-verify.sh b/test/sharness/t0265-filestore-verify.sh
@@ -163,7 +163,7 @@ interesting_prep() {
     ipfs filestore rm $E_HASH
   '
 
-  test_expect_failure "'filestore verify' produces expected output" '
+  test_expect_success "'filestore verify' produces expected output" '
     cp verify-initial verify-now &&
     cmp_verify
   '
@@ -185,7 +185,7 @@ ok       QmaVeSKhGmPYxRyqA236Y4N5e4Rn6LGZKdCgaYUarEo5Nu
 
 ok       QmcAkMdfBPYVzDCM6Fkrz1h8WXcprH8BLF6DmjNUGhXAnm
 EOF
-test_expect_failure "'filestore clean orphan' (should remove 'changed' orphan)" '
+test_expect_success "'filestore clean orphan' (should remove 'changed' orphan)" '
   ipfs filestore clean orphan &&
   cmp_verify
 '
@@ -205,7 +205,7 @@ orphan   QmYswupx1AdGdTn6GeXVdaUBEe6rApd7GWSnobcuVZjeRV
 orphan   QmfDSgGhGsEf7LHC6gc7FbBMhGuYzxTLnbKqFBkWhGt8Qp
 orphan   QmSWnPbrLFmxfJ9vj2FvKKpVmu3SZprbt7KEbkUVjy7bMD
 EOF
-test_expect_failure "'filestore clean incomplete' (will create more orphans)" '
+test_expect_success "'filestore clean incomplete' (will create more orphans)" '
   ipfs filestore clean incomplete &&
   cmp_verify
 '
@@ -221,7 +221,7 @@ ok       QmaVeSKhGmPYxRyqA236Y4N5e4Rn6LGZKdCgaYUarEo5Nu
 
 ok       QmcAkMdfBPYVzDCM6Fkrz1h8WXcprH8BLF6DmjNUGhXAnm
 EOF
-test_expect_failure "'filestore clean orphan'" '
+test_expect_success "'filestore clean orphan'" '
   ipfs filestore clean orphan &&
   cmp_verify
 '
@@ -238,7 +238,7 @@ orphan   QmbZr7Fs6AJf7HpnTxDiYJqLXWDqAy3fKFXYVDkgSsH7DH
 orphan   QmToAcacDnpqm17jV7rRHmXcS9686Mk59KCEYGAMkh9qCX
 orphan   QmYtLWUVmevucXFN9q59taRT95Gxj5eJuLUhXKtwNna25t
 EOF
-test_expect_failure "'filestore clean changed incomplete' (will create more orphans)" '
+test_expect_success "'filestore clean changed incomplete' (will create more orphans)" '
   ipfs filestore clean changed incomplete &&
   cmp_verify
 '
@@ -255,7 +255,7 @@ orphan   QmToAcacDnpqm17jV7rRHmXcS9686Mk59KCEYGAMkh9qCX
 orphan   QmbZr7Fs6AJf7HpnTxDiYJqLXWDqAy3fKFXYVDkgSsH7DH
 orphan   QmYtLWUVmevucXFN9q59taRT95Gxj5eJuLUhXKtwNna25t
 EOF
-test_expect_failure "'filestore clean no-file' (will create an incomplete)" '
+test_expect_success "'filestore clean no-file' (will create an incomplete)" '
   ipfs filestore clean no-file &&
   cmp_verify
 '
@@ -265,7 +265,7 @@ ok       QmaVeSKhGmPYxRyqA236Y4N5e4Rn6LGZKdCgaYUarEo5Nu
 
 ok       QmcAkMdfBPYVzDCM6Fkrz1h8WXcprH8BLF6DmjNUGhXAnm
 EOF
-test_expect_failure "'filestore clean incomplete orphan' (cleanup)" '
+test_expect_success "'filestore clean incomplete orphan' (cleanup)" '
   cp verify-final verify-now &&
   ipfs filestore clean incomplete orphan &&
   cmp_verify
@@ -277,7 +277,7 @@ test_expect_failure "'filestore clean incomplete orphan' (cleanup)" '
 
 interesting_prep
 
-test_expect_failure "'filestore clean full'" '
+test_expect_success "'filestore clean full'" '
   cp verify-final verify-now &&
   ipfs filestore clean full &&
   cmp_verify

diff --git a/test/sharness/t0266-filestore-concurrent.sh b/test/sharness/t0266-filestore-concurrent.sh
@@ -75,11 +75,11 @@ test_expect_success "filestore clean invalid race condation" '(
   wait
 )'
 
-test_expect_failure "filestore clean race condation: output looks good" '
+test_expect_success "filestore clean race condation: output looks good" '
   grep "cannot remove $HASH" clean-actual
 '
 
-test_expect_failure "filestore clean race condation: file still available" '
+test_expect_success "filestore clean race condation: file still available" '
   ipfs cat "$HASH" > /dev/null
 '