Skip to content
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.

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

swarm/network/stream: added pure retrieval test (syncing disabled) #1355

Open
wants to merge 3 commits into
base: swarm-rather-stable
from

Conversation

Projects
2 participants
@holisticode
Copy link

commented Apr 26, 2019

his PR adds a pure retrieval test to the snapshot retrieval tests.

Up to today, the retrieval tests were uploading chunks randomly to nodes and then trying to download those chunks from every node. To upload chunks, the FileStore.Store() function was being used - essentially uploading chunks to individual nodes directly. For retrieval to work in such a scenario, we need to have syncing enabled in order for chunks to be stored at their actual expected nodes.

This new test bypasses syncing by evaluating which chunk would be expected to be at which node, then storing chunks directly in those nodes's LocalStore - essentially simulating syncing by accessing stores directly (simulation framework allows that). This way, the test simulation run can switch off syncing and directly starts retrieving chunks, as if syncing would have already terminated.

@zelig zelig added this to Backlog in Swarm via automation Apr 29, 2019

@zelig zelig moved this from Backlog to In review in Swarm Apr 29, 2019

Show resolved Hide resolved swarm/network/stream/snapshot_retrieval_test.go Outdated
Show resolved Hide resolved swarm/network/stream/snapshot_retrieval_test.go Outdated
Show resolved Hide resolved swarm/network/stream/snapshot_retrieval_test.go
log.Info("Starting simulation")

result := sim.Run(ctx, func(ctx context.Context, sim *simulation.Simulation) error {
nodeIDs := sim.UpNodeIDs()

This comment has been minimized.

Copy link
@zelig

zelig May 1, 2019

why not put this part outside the simulation Run just as part of setup?

This comment has been minimized.

Copy link
@holisticode

holisticode May 3, 2019

Author

Strictly speaking, if we want the UpNodeIDs() (the nodes which are Up), then we should query this information when the simulation is started, thus inside sim.Run()

Show resolved Hide resolved swarm/network/stream/snapshot_retrieval_test.go Outdated
cnt := 0

REPEAT:
for {

This comment has been minimized.

Copy link
@zelig

zelig May 1, 2019

so this test suits the old action, trigger, check pattern

This comment has been minimized.

Copy link
@zelig

zelig May 1, 2019

that would also make it concurrent, which is also better for testing realistic scenarios.
I would just allocate chunks to nodes to retrieve them independently, otherwise caching may distort the pure nature of this test

This comment has been minimized.

Copy link
@holisticode

holisticode May 3, 2019

Author

I am not sure I understand what you mean here and I am also not sure what action I should take on your comments. I have thus currently not changed anything

return fmt.Errorf("No filestore")
}
fileStore := item.(*storage.FileStore)
for _, chunk := range chunks {

This comment has been minimized.

Copy link
@zelig

zelig May 1, 2019

if we retrieve each chunk from each node, there is a lot of caching happening, so it may shield retrieval issues

This comment has been minimized.

Copy link
@holisticode

holisticode May 3, 2019

Author

In some way I agree but I finally disagree.

Retrieval in Swarm is intrinsically linked to caching. This is something you yourself have been raising all the time, when saying that we actually can't switch off caching.

So my conclusion is that this in turn is in fact also part of retrieval, so we should leave it as-is. I would furthermore not even know how to circumvent this situation, and finally, you should be able to specify what you mean by "retrieval issues" in order to do something about it.

for _, chunk := range chunks {
reader, _ := fileStore.Retrieve(context.TODO(), chunk.Address())
//check that we can read the file size and that it corresponds to the generated file size
if s, err := reader.Size(ctx, nil); err != nil || s != int64(chunkSize) {

This comment has been minimized.

Copy link
@zelig

zelig May 1, 2019

do we need this part?
or why check size only?

Show resolved Hide resolved swarm/network/stream/snapshot_retrieval_test.go Outdated

// second iteration: store chunks at the nodes they would be
// expected to be
log.Debug("storing every chunk at correspondent node store")

This comment has been minimized.

Copy link
@zelig

zelig May 1, 2019

"storing chunk at the peer whose overlay address is closest to chunk hash`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.