Skip to content

Local pinning fix #255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jmozah merged 5 commits into from
Jun 9, 2020
Merged

Local pinning fix #255

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e39afef
Added conditions wherever we add top gcIndex
Jun 4, 2020
7edb3d6
Add check for pinIndex wherever we add a chyunk in gcIndex
Jun 4, 2020
3f502f4
removed debug fmt statements
Jun 5, 2020
9fb5ad3
mreview fixes
Jun 8, 2020
8392a05
Removed debug statements and parameterized few test cases
Jun 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
191 changes: 191 additions & 0 deletions pkg/localstore/gc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"bytes"
"context"
"errors"
"fmt"
"io/ioutil"
"math/rand"
"os"
Expand Down Expand Up @@ -534,3 +535,193 @@ func TestSetTestHookCollectGarbage(t *testing.T) {
t.Errorf("got hook value %v, want %v", got, original)
}
}

func TestPinAfterMultiGC(t *testing.T) {
db := newTestDB(t, &Options{
Capacity: 100,
})

pinnedChunks := make([]swarm.Address, 0)

// upload random chunks above db capacity to see if chunks are still pinned
for i := 0; i < 200; i++ {
ch := generateTestRandomChunk()
_, err := db.Put(context.Background(), storage.ModePutUpload, ch)
if err != nil {
t.Fatal(err)
}
err = db.Set(context.Background(), storage.ModeSetSyncPull, ch.Address())
if err != nil {
t.Fatal(err)
}

if len(pinnedChunks) < 10 {
Comment thread
acud marked this conversation as resolved.
rch := generateAndPinAChunk(t, db)
pinnedChunks = append(pinnedChunks, rch.Address())
}
}
for i := 0; i < 200; i++ {
ch := generateTestRandomChunk()
_, err := db.Put(context.Background(), storage.ModePutUpload, ch)
if err != nil {
t.Fatal(err)
}
err = db.Set(context.Background(), storage.ModeSetSyncPull, ch.Address())
if err != nil {
t.Fatal(err)
}
}
for i := 0; i < 200; i++ {
ch := generateTestRandomChunk()
_, err := db.Put(context.Background(), storage.ModePutUpload, ch)
if err != nil {
t.Fatal(err)
}
err = db.Set(context.Background(), storage.ModeSetSyncPull, ch.Address())
if err != nil {
t.Fatal(err)
}
}

t.Run("pin Index count", newItemsCountTest(db.pinIndex, len(pinnedChunks)))

// Check if all the pinned chunks are present in the data DB
for _, addr := range pinnedChunks {
outItem := shed.Item{
Address: addr.Bytes(),
}
gotChunk, err := db.Get(context.Background(), storage.ModeGetRequest, swarm.NewAddress(outItem.Address))
if err != nil {
t.Fatal(err)
}
if !gotChunk.Address().Equal(swarm.NewAddress(addr.Bytes())) {
t.Fatal("Pinned chunk is not equal to got chunk")
}
}

}

func generateAndPinAChunk(t *testing.T, db *DB) swarm.Chunk {
// Create a chunk and pin it
pinnedChunk := generateTestRandomChunk()

_, err := db.Put(context.Background(), storage.ModePutUpload, pinnedChunk)
if err != nil {
t.Fatal(err)
}
err = db.Set(context.Background(), storage.ModeSetPin, pinnedChunk.Address())
if err != nil {
t.Fatal(err)
}
err = db.Set(context.Background(), storage.ModeSetSyncPull, pinnedChunk.Address())
if err != nil {
t.Fatal(err)
}
return pinnedChunk
}

func TestPinSyncAndAccessPutSetChunkMultipleTimes(t *testing.T) {
var closed chan struct{}
testHookCollectGarbageChan := make(chan uint64)
t.Cleanup(setTestHookCollectGarbage(func(collectedCount uint64) {
select {
case testHookCollectGarbageChan <- collectedCount:
case <-closed:
}
}))
db := newTestDB(t, &Options{
Capacity: 10,
})
closed = db.close

rch := generateTestRandomChunk()
for i := 0; i < 20; i++ {
ch := generateTestRandomChunk()

// pin a chunk in between
if i == 5 {
_, err := db.Put(context.Background(), storage.ModePutUpload, rch)
if err != nil {
t.Fatal(err)
}
err = db.Set(context.Background(), storage.ModeSetPin, rch.Address())
if err != nil {
t.Fatal(err)
}
continue
}

_, err := db.Put(context.Background(), storage.ModePutUpload, ch)
if err != nil {
t.Fatal(err)
}
err = db.Set(context.Background(), storage.ModeSetSyncPull, ch.Address())
if err != nil {
t.Fatal(err)
}

err = db.Set(context.Background(), storage.ModeSetAccess, ch.Address())
if err != nil {
t.Fatal(err)
}
_, err = db.Get(context.Background(), storage.ModeGetRequest, ch.Address())
if err != nil {
t.Fatal(err)
}
}

_, err := db.Get(context.Background(), storage.ModeGetRequest, rch.Address())
if err != nil {
fmt.Println("Pinned chunk missing ", rch.Address())
t.Fatal(err)
}

addRandomChunks(t, 20, db)

_, err = db.Get(context.Background(), storage.ModeGetRequest, rch.Address())
if err != nil {
fmt.Println("Pinned chunk missing ", rch.Address())
t.Fatal(err)
}

addRandomChunks(t, 20, db)

_, err = db.Get(context.Background(), storage.ModeGetRequest, rch.Address())
if err != nil {
fmt.Println("Pinned chunk missing ", rch.Address())

@acud acud Jun 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again and again we ask to remove fmt.Println from the codebase. Can we please agree that you review your own code and get rid of such trivial mistakes that consistently come up in every review?

@jmozah jmozah Jun 9, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops... my bad.. will remove it

t.Fatal(err)
}

// check if the pinned chunk is present after GC
gotChunk, err := db.Get(context.Background(), storage.ModeGetRequest, rch.Address())
if err != nil {
t.Fatal("Pinned chunk missing ", err)
}
if !gotChunk.Address().Equal(rch.Address()) {
t.Fatal("Pinned chunk is not equal to got chunk")
}

}

func addRandomChunks(t *testing.T, count int, db *DB) {

@acud acud Jun 5, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't you use this in all of the tests above?

@jmozah jmozah Jun 8, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because other tests dont need this.

@acud acud Jun 9, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm seeing a lot of Puts and Sets above this, if this test helper would have a few parameters, it could save you all of the manual Puts and Sets

for i := 0; i < count; i++ {
ch := generateTestRandomChunk()
_, err := db.Put(context.Background(), storage.ModePutUpload, ch)
if err != nil {
t.Fatal(err)
}
err = db.Set(context.Background(), storage.ModeSetSyncPull, ch.Address())
if err != nil {
t.Fatal(err)
}
err = db.Set(context.Background(), storage.ModeSetAccess, ch.Address())
if err != nil {
t.Fatal(err)
}
_, err = db.Get(context.Background(), storage.ModeGetRequest, ch.Address())
if err != nil {
t.Fatal(err)
}

}
}
2 changes: 1 addition & 1 deletion pkg/localstore/localstore_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,6 @@ func TestDBDebugIndexes(t *testing.T) {
}

// assert that there's a pin and gc exclude entry now
testIndexCounts(t, 1, 1, 1, 1, 1, 1, 1, indexCounts)
testIndexCounts(t, 1, 1, 0, 1, 1, 1, 1, indexCounts)
Comment thread
acud marked this conversation as resolved.

}
15 changes: 12 additions & 3 deletions pkg/localstore/mode_get.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@ import (
"errors"
"time"

"github.com/syndtr/goleveldb/leveldb"

"github.com/ethersphere/bee/pkg/shed"
"github.com/ethersphere/bee/pkg/storage"
"github.com/ethersphere/bee/pkg/swarm"
"github.com/syndtr/goleveldb/leveldb"
)

// Get returns a chunk from the database. If the chunk is
Expand Down Expand Up @@ -155,11 +156,19 @@ func (db *DB) updateGC(item shed.Item) (err error) {
if err != nil {
return err
}
// add new entry to gc index
err = db.gcIndex.PutInBatch(batch, item)

// add new entry to gc index ONLY if it is not present in pinIndex
ok, err := db.pinIndex.Has(item)
if err != nil {
return err
}
if !ok {
err = db.gcIndex.PutInBatch(batch, item)
if err != nil {
return err
}
}

return db.shed.WriteBatch(batch)
}

Expand Down
12 changes: 9 additions & 3 deletions pkg/localstore/mode_put.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,12 +338,18 @@ func (db *DB) setGC(batch *leveldb.Batch, item shed.Item) (gcSizeChange int64, e
return 0, err
}

err = db.gcIndex.PutInBatch(batch, item)
// add new entry to gc index ONLY if it is not present in pinIndex
ok, err := db.pinIndex.Has(item)
if err != nil {
return 0, err
}

gcSizeChange++
if !ok {
err = db.gcIndex.PutInBatch(batch, item)
if err != nil {
return 0, err
}
gcSizeChange++
}

return gcSizeChange, nil
}
Expand Down
14 changes: 11 additions & 3 deletions pkg/localstore/mode_set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@ import (
"errors"
"time"

"github.com/syndtr/goleveldb/leveldb"

"github.com/ethersphere/bee/pkg/storage"
"github.com/ethersphere/bee/pkg/swarm"
"github.com/ethersphere/bee/pkg/tags"
"github.com/syndtr/goleveldb/leveldb"
)

// Set updates database indexes for
Expand Down Expand Up @@ -180,11 +181,18 @@ func (db *DB) setAccess(batch *leveldb.Batch, binIDs map[uint8]uint64, addr swar
if err != nil {
return 0, err
}
err = db.gcIndex.PutInBatch(batch, item)

ok, err := db.pinIndex.Has(item)
if err != nil {
return 0, err
}
gcSizeChange++
if !ok {
err = db.gcIndex.PutInBatch(batch, item)
if err != nil {
return 0, err
}
gcSizeChange++
}

return gcSizeChange, nil
}
Expand Down