Repository: dgraph-io/badger
Pull request #357 (2017-12-13 05:48:47)

Run gc until we encounter non-nil error.
Earlier we ran the GC only once, relying on the user to call
DB.RunValueLogGC() multiple times.  We now record some discard
statistics to the vlog struct. We can use that information to improve
the GC.

This commit runs the GC multiple times, picking a different log file
each time, until we encounter an error. We expect to stop the GC the
moment we receive a ErrNoRewrite error.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/dgraph-io/badger/357)
<!-- Reviewable:end -->


manishrjain: 



Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

*[value.go, line 1036 at r1](https://reviewable.io:443/reviews/dgraph-io/badger/357#-L0DYQmu-qAu566nQxz7:-L0DYQmu-qAu566nQxz8:blmpsx5) ([raw file](https://github.com/dgraph-io/badger/blob/9e27da577a8ee96497843c33537efcdaca182e0e/value.go#L1036)):*
> ```Go
> 		}
> 		<-vlog.garbageCh
> 		if err == ErrNoRewrite {
> ```

If we had at least one successful GC, we should not return this.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/dgraph-io/badger/357)*
<!-- Sent from Reviewable.io -->

deepakjois: 



Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.

---

*[value.go, line 1036 at r1](https://reviewable.io:443/reviews/dgraph-io/badger/357#-L0DYQmu-qAu566nQxz7:-L0DbKle2ejqjL8aZ8-l:b-5ub9mp) ([raw file](https://github.com/dgraph-io/badger/blob/9e27da577a8ee96497843c33537efcdaca182e0e/value.go#L1036)):*
<details><summary><i>Previously, manishrjain (Manish R Jain) wrote…</i></summary><blockquote>

If we had at least one successful GC, we should not return this.
</blockquote></details>

I am not entirely sure what you mean. But I have tweaked this code. I am keeping track of the number of successful GCs. If this `count > 0` then we return nil. If the count is 0, then we return `ErrRewrite`. This does a better job of preserving the previous semantics.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/dgraph-io/badger/357)*
<!-- Sent from Reviewable.io -->

manishrjain: <img class="emoji" title=":lgtm:" alt=":lgtm:" align="absmiddle" src="https://reviewable.io/lgtm.png" height="20" width="61"/>

---

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

*[value.go, line 1036 at r1](https://reviewable.io:443/reviews/dgraph-io/badger/357#-L0DYQmu-qAu566nQxz7:-L0DfKk7YGmPIyxf4gTH:b-85i63v) ([raw file](https://github.com/dgraph-io/badger/blob/9e27da577a8ee96497843c33537efcdaca182e0e/value.go#L1036)):*
<details><summary><i>Previously, deepakjois (Deepak Jois) wrote…</i></summary><blockquote>

I am not entirely sure what you mean. But I have tweaked this code. I am keeping track of the number of successful GCs. If this `count > 0` then we return nil. If the count is 0, then we return `ErrRewrite`. This does a better job of preserving the previous semantics.
</blockquote></details>

LGTM.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/dgraph-io/badger/357#-:-L0DfNr13lT3Lr3ZP3BE:bnfp4nl)*
<!-- Sent from Reviewable.io -->

janardhan1993: <img class="emoji" title=":lgtm:" alt=":lgtm:" align="absmiddle" src="https://reviewable.io/lgtm.png" height="20" width="61"/>

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

*[value.go, line 1033 at r2](https://reviewable.io:443/reviews/dgraph-io/badger/357#-L0Dg_P3e4e7noIgUH30:-L0Dg_P44JkCyvHl0p0a:b-vjf59o) ([raw file](https://github.com/dgraph-io/badger/blob/26e500f34f0105639ffa12a9ae6a1f816d272ae8/value.go#L1033)):*
> ```Go
> 		)
> 		for {
> 			err = vlog.doRunGC(gcThreshold, head)
> ```

Doing all files at once might increase the disk io depending on when GC is triggered by user. Shouldn't be an issue if user triggers GC periodically instead of waiting for long time.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/dgraph-io/badger/357#-:-L0Dh3mCPdip01bl8LXv:bnfp4nl)*
<!-- Sent from Reviewable.io -->

deepakjois: 



Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.

---

*[value.go, line 1033 at r2](https://reviewable.io:443/reviews/dgraph-io/badger/357#-L0Dg_P3e4e7noIgUH30:-L0E3c1DciCYjU4urNkn:b-ffx4va) ([raw file](https://github.com/dgraph-io/badger/blob/26e500f34f0105639ffa12a9ae6a1f816d272ae8/value.go#L1033)):*
<details><summary><i>Previously, janardhan1993 (Janardhan Reddy) wrote…</i></summary><blockquote>

Doing all files at once might increase the disk io depending on when GC is triggered by user. Shouldn't be an issue if user triggers GC periodically instead of waiting for long time.
</blockquote></details>

Ok. I will make a note of that in the docs.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/dgraph-io/badger/357)*
<!-- Sent from Reviewable.io -->


Patch:
diff --git a/README.md b/README.md
index 3d1102e..cdaea43 100644
--- a/README.md
+++ b/README.md
@@ -361,11 +361,16 @@ garbage collection.
 * `DB.PurgeVersionsBelow(key, ts)`: This method is useful to do a more targeted clean up of older versions
 of key-value pairs. You can specify a key, and a timestamp. All versions of the key older than the timestamp
 are marked as deleted, making them eligible for garbage collection.
-* `DB.RunValueLogGC()`: This method triggers a value log garbage collection for a single log file. There
-are no guarantees that a call would result in space reclamation. Every run would rewrite at most one log
-file. So, repeated calls may be necessary. Please ensure that you call the `DB.Purge…()` methods first
-before invoking this method.
-
+* `DB.RunValueLogGC()`: This method is designed to do garbage collection while
+  Badger is online. Please ensure that you call the `DB.Purge…()` methods first
+  before invoking this method. It uses any statistics generated by the
+  `DB.Purge(…)` methods to pick files that are likely to lead to maximum space
+  reclamation. It loops until it encounters a file which does not lead to any
+  garbage collection.
+
+  It could lead to increased I/O if `DB.RunValueLogGC()` hasn’t been called for
+  a long time, and many deletes have happened in the meanwhile. So it is recommended
+  that this method be called regularly.
 
 ### Database backup
 There are two public API methods `DB.Backup()` and `DB.Load()` which can be
diff --git a/value.go b/value.go
index c53d1b9..1108010 100644
--- a/value.go
+++ b/value.go
@@ -1024,10 +1024,22 @@ func (vlog *valueLog) waitOnGC(lc *y.Closer) {
 func (vlog *valueLog) runGC(gcThreshold float64, head valuePointer) error {
 	select {
 	case vlog.garbageCh <- struct{}{}:
-
 		// Run GC
-		err := vlog.doRunGC(gcThreshold, head)
+		var (
+			err   error
+			count int
+		)
+		for {
+			err = vlog.doRunGC(gcThreshold, head)
+			if err != nil {
+				break
+			}
+			count++
+		}
 		<-vlog.garbageCh
+		if err == ErrNoRewrite && count > 0 {
+			return nil
+		}
 		return err
 	default:
 		return ErrRejected


Test patch:
diff --git a/value_test.go b/value_test.go
index 7fae7f5..8f1b335 100644
--- a/value_test.go
+++ b/value_test.go
@@ -554,7 +554,9 @@ func TestValueLogGC(t *testing.T) {
 		require.NoError(t, kv.PurgeOlderVersions())
 		require.NoError(t, kv.RunValueLogGC(0.3))
 		newFids := kv.vlog.sortedFids()
-		require.Equal(t, len(newFids)+1, len(fids))
+		// No. of value log files after GC should be less than before.
+		// We should have GC-ed more than one value log file.
+		require.True(t, (len(fids)-len(newFids)) > 2)
 		for i, fid := range fids {
 			if i < len(newFids) && newFids[i] == fid {
 				continue