Make KeyValueStore.Delete report if the key to delete was found.

Previously, it would return an error instead. Now we can distinguish
the cases 'error while deleting known key' vs. 'key not in index'
without testing for leveldb-internal kinds of errors.

storage/local/index/interface.go

@@ -29,8 +29,8 @@ type KeyValueStore interface {
 	// could be found for key. If value is nil, Get behaves like Has.
 	Get(key encoding.BinaryMarshaler, value encoding.BinaryUnmarshaler) (bool, error)
 	Has(key encoding.BinaryMarshaler) (bool, error)
-	// Delete returns an error if key does not exist.
+	// Delete returns (false, nil) if key does not exist.
-	Delete(key encoding.BinaryMarshaler) error
+	Delete(key encoding.BinaryMarshaler) (bool, error)
 
 	NewBatch() Batch
 	Commit(b Batch) error

storage/local/index/leveldb.go

@@ -104,12 +104,19 @@ func (l *LevelDB) Has(key encoding.BinaryMarshaler) (has bool, err error) {
 }
 
 // Delete implements KeyValueStore.
-func (l *LevelDB) Delete(key encoding.BinaryMarshaler) error {
+func (l *LevelDB) Delete(key encoding.BinaryMarshaler) (bool, error) {
 	k, err := key.MarshalBinary()
 	if err != nil {
-		return err
+		return false, err
 	}
-	return l.storage.Delete(k, l.writeOpts)
+	err = l.storage.Delete(k, l.writeOpts)
+	if err == leveldb.ErrNotFound {
+		return false, nil
+	}
+	if err != nil {
+		return false, err
+	}
+	return true, nil
 }
 
 // Put implements KeyValueStore.

storage/local/persistence.go

@@ -535,16 +535,13 @@ func (p *persistence) cleanUpArchiveIndexes(
 			if !fpSeen {
 				glog.Warningf("Archive clean-up: Fingerprint %v is unknown. Purging from archive indexes.", clientmodel.Fingerprint(fp))
 			}
-			if err := p.archivedFingerprintToMetrics.Delete(fp); err != nil {
+			// It's fine if the fp is not in the archive indexes.
+			if _, err := p.archivedFingerprintToMetrics.Delete(fp); err != nil {
 				return err
 			}
 			// Delete from timerange index, too.
-			p.archivedFingerprintToTimeRange.Delete(fp)
+			_, err := p.archivedFingerprintToTimeRange.Delete(fp)
-			// TODO: Ignoring errors here as fp might not be in
+			return err
-			// timerange index (which is good) but which would
-			// return an error. Delete signature could be changed
-			// like the Get signature to detect a real error.
-			return nil
 		}
 		// fp is legitimately archived. Make sure it is in timerange index, too.
 		has, err := p.archivedFingerprintToTimeRange.Has(fp)
@@ -555,7 +552,8 @@ func (p *persistence) cleanUpArchiveIndexes(
 			return nil // All good.
 		}
 		glog.Warningf("Archive clean-up: Fingerprint %v is not in time-range index. Unarchiving it for recovery.")
-		if err := p.archivedFingerprintToMetrics.Delete(fp); err != nil {
+		// Again, it's fine if fp is not in the archive index.
+		if _, err := p.archivedFingerprintToMetrics.Delete(fp); err != nil {
 			return err
 		}
 		if err := kv.Value(&m); err != nil {
@@ -590,9 +588,13 @@ func (p *persistence) cleanUpArchiveIndexes(
 			return nil // All good.
 		}
 		glog.Warningf("Archive clean-up: Purging unknown fingerprint %v in time-range index.", fp)
-		if err := p.archivedFingerprintToTimeRange.Delete(fp); err != nil {
+		deleted, err := p.archivedFingerprintToTimeRange.Delete(fp)
+		if err != nil {
 			return err
 		}
+		if !deleted {
+			glog.Errorf("Fingerprint %s to be deleted from archivedFingerprintToTimeRange not found. This should never happen.", fp)
+		}
 		return nil
 	}); err != nil {
 		return err
@@ -1303,12 +1305,20 @@ func (p *persistence) dropArchivedMetric(fp clientmodel.Fingerprint) (err error)
 	if err != nil || metric == nil {
 		return err
 	}
-	if err := p.archivedFingerprintToMetrics.Delete(codable.Fingerprint(fp)); err != nil {
+	deleted, err := p.archivedFingerprintToMetrics.Delete(codable.Fingerprint(fp))
+	if err != nil {
 		return err
 	}
-	if err := p.archivedFingerprintToTimeRange.Delete(codable.Fingerprint(fp)); err != nil {
+	if !deleted {
+		glog.Errorf("Tried to delete non-archived fingerprint %s from archivedFingerprintToMetrics index. This should never happen.", fp)
+	}
+	deleted, err = p.archivedFingerprintToTimeRange.Delete(codable.Fingerprint(fp))
+	if err != nil {
 		return err
 	}
+	if !deleted {
+		glog.Errorf("Tried to delete non-archived fingerprint %s from archivedFingerprintToTimeRange index. This should never happen.", fp)
+	}
 	p.unindexMetric(fp, metric)
 	return nil
 }
@@ -1332,12 +1342,20 @@ func (p *persistence) unarchiveMetric(fp clientmodel.Fingerprint) (
 	if err != nil || !has {
 		return false, firstTime, err
 	}
-	if err := p.archivedFingerprintToMetrics.Delete(codable.Fingerprint(fp)); err != nil {
+	deleted, err := p.archivedFingerprintToMetrics.Delete(codable.Fingerprint(fp))
+	if err != nil {
 		return false, firstTime, err
 	}
-	if err := p.archivedFingerprintToTimeRange.Delete(codable.Fingerprint(fp)); err != nil {
+	if !deleted {
+		glog.Errorf("Tried to delete non-archived fingerprint %s from archivedFingerprintToMetrics index. This should never happen.", fp)
+	}
+	deleted, err = p.archivedFingerprintToTimeRange.Delete(codable.Fingerprint(fp))
+	if err != nil {
 		return false, firstTime, err
 	}
+	if !deleted {
+		glog.Errorf("Tried to delete non-archived fingerprint %s from archivedFingerprintToTimeRange index. This should never happen.", fp)
+	}
 	return true, firstTime, nil
 }