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

[bee #48, #49, #40]: introduce netstore #104

Merged
merged 2 commits into from
Apr 21, 2020
Merged

[bee #48, #49, #40]: introduce netstore #104

merged 2 commits into from
Apr 21, 2020

Conversation

acud
Copy link
Member

@acud acud commented Apr 16, 2020

Due to popular demand - the return of the immortal NetStore

This is a naive and trivial NetStore implementation: no caching and no fancy stuff, including any concurrency.

A note about about the retrieval protocol:
I introduced the Interface type but I cannot change the New constructor to return the Interface type, that is because in the retrieval tests, the server is expected to implement the Protocol method:

   recorder := streamtest.New(
   	streamtest.WithProtocols(server.Protocol()),
   )

Personally it would be nice to return the interface directly from the constructor and not even export Service at all, but then the test won't fly in this form.
It does work when I cast server to a Service:

	sprot := server.(*retrieval.Service)
	recorder := streamtest.New(
		streamtest.WithProtocols(sprot.Protocol()),
	)

But I feel this solution is also sub-optimal since Service still needs to be exported.
If someone has some nicer idea, I'd be happy to hear.

needed for #48
closes #49
closes #40
related to #98

@acud acud added the in progress ongoing development , hold out with review label Apr 16, 2020
@acud acud self-assigned this Apr 16, 2020
@acud acud changed the title [bee #48, 49]: introduce netstore [bee #48, #49, #40]: introduce netstore Apr 16, 2020
@acud acud added ready for review The PR is ready to be reviewed and removed in progress ongoing development , hold out with review labels Apr 17, 2020
@acud acud added in progress ongoing development , hold out with review ready for review The PR is ready to be reviewed and removed ready for review The PR is ready to be reviewed in progress ongoing development , hold out with review labels Apr 17, 2020
@pradovic
Copy link
Contributor

pradovic commented Apr 20, 2020

@acud You can always export something (constructor returning Service, for ex) only for tests in export_test.go. But how would you use Service if it is not exported? It is needed to add protocol to p2p, right? Sorry if I confused something.


type mockValidator struct{}

func (m mockValidator) Validate(_ swarm.Chunk) bool { return true }
Copy link
Member

Choose a reason for hiding this comment

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

An extremely minor suggestion, m receiver is not needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

but it has to implement the interface (the validator is an interface, not a function type alias)

Copy link
Member

Choose a reason for hiding this comment

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

This is very very minor, but just as swarm.Chunk is _, func (_ mockValidator) ... can have _.

Copy link
Contributor

@pradovic pradovic left a comment

Choose a reason for hiding this comment

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

Lgtm 👏

@acud acud merged commit 052f751 into master Apr 21, 2020
@acud acud deleted the netstore branch April 21, 2020 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Download fetches from closest node Make sure chunk is valid upon upload
3 participants