feat(api): use new storagev2 interfaces #3833
Conversation
| // Use of this source code is governed by a BSD-style | ||
| // license that can be found in the LICENSE file. | ||
|
|
||
| package mockstorer |
There was a problem hiding this comment.
why do we need a mock storer? Wouldn't it be better to use the real thing?
There was a problem hiding this comment.
Usually, for unit tests we mock the objects that the package uses and are not defined in the package. The storer should be tested separately. Having API pkg unit tests depend on the actual storer implementation correctness is not right. There could be bugs in the implementation etc. The goal of the unit tests here is just to ensure the code in the api pkg files is correct.
The type of tests you are mentioning are integration tests. We can have these separately. Or we can develop beekeeper tests that will do this. We will probably have to change a bunch of those anyway, so we can even add some. Along with this, we need to provide debug APIs for showing storer state to verify in the tests.
There was a problem hiding this comment.
Having API pkg unit tests depend on the actual storer implementation correctness is not right. There could be bugs in the implementation etc
I don't understand this. We are not mocking here something complex like kademlia that's needs external connections.
We are mocking something that can be created as easily as storer.New(ctx context.Context, "", opts)
But I get it, we don't really want to spend the energy modifying already existing tests.
aloknerurkar
force-pushed
the
localstorev2.0-integration
branch
from
66a2b3c
to
d4ccaf7
Compare
| // Iterate over chunks in no particular order. | ||
| Iterate(context.Context, IterateChunkFn) error | ||
| } | ||
|
|
||
| type ReadOnlyChunkStore interface { | ||
| Getter |
There was a problem hiding this comment.
Right now this interface is just being used to access the local chunkstore. We dont want to manage the chunkstore on the caller side. So closer would anyway be a noop.
| @@ -353,25 +348,18 @@ func (s *Service) Close() error { | |||
|
|
|||
| // getOrCreateTag attempts to get the tag if an id is supplied, and returns an error if it does not exist. | |||
| // If no id is supplied, it will attempt to create a new tag with a generated name and return it. | |||
| func (s *Service) getOrCreateTag(tagUid string) (*tags.Tag, bool, error) { | |||
| func (s *Service) getOrCreateSessionID(tagUid uint64) (uint64, error) { | |||
There was a problem hiding this comment.
You can register this as a hook in preMapHooks similar to the resolveNameOrAddress method. That way you'll just use the tag on the struct.
There was a problem hiding this comment.
So the problem is the usage is different in different APIs. For eg we create a tag only when deferred upload/pin flag is set in header. But this is not always true, sometime we will create a tag by default. Can this be handled in premaphook?
pkg/steward/steward.go
Outdated
| return err | ||
| } | ||
|
|
||
| err = uploaderSession.Put(ctx, c) |
There was a problem hiding this comment.
No need to check the error, the following will do: return uploaderSession.Put(ctx, c).
Checklist
Description
This change is