From 73f6dc4d44e32f814105a8abca9fffd7b6080edf Mon Sep 17 00:00:00 2001 From: Bjoern Rabenstein Date: Thu, 29 Jan 2015 12:57:50 +0100 Subject: [PATCH] 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 | 4 +-- storage/local/index/leveldb.go | 13 +++++++--- storage/local/persistence.go | 44 ++++++++++++++++++++++---------- 3 files changed, 43 insertions(+), 18 deletions(-) diff --git a/storage/local/index/interface.go b/storage/local/index/interface.go index 9b79ab2ded..40080c7f35 100644 --- a/storage/local/index/interface.go +++ b/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(key encoding.BinaryMarshaler) error + // Delete returns (false, nil) if key does not exist. + Delete(key encoding.BinaryMarshaler) (bool, error) NewBatch() Batch Commit(b Batch) error diff --git a/storage/local/index/leveldb.go b/storage/local/index/leveldb.go index 77bed8abce..4a1345ab3a 100644 --- a/storage/local/index/leveldb.go +++ b/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. diff --git a/storage/local/persistence.go b/storage/local/persistence.go index a0beff2e5e..7691409e66 100644 --- a/storage/local/persistence.go +++ b/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) - // TODO: Ignoring errors here as fp might not be in - // 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 + _, err := p.archivedFingerprintToTimeRange.Delete(fp) + return err } // 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 }