[spaces 1/2] initial ListStorageSpaces implementation #1802
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
|
@butonic, can you restart the CI build or do I need |
|
@C0rby I can't, but pushing should trigger it automatically. |
There was a problem hiding this comment.
I would like for the Reva code base to be more approachable and part of it is to start being consistent. During this review I had this in mind:
- focus in consistency (i.e
varblocks vs inline declarations) - special emphasis on reducing the cyclomatic complexity of certain functions
- this includes:
- refactoring
- abstracting function bodies away (not refactoring per-se...)
- this includes:
- clarity of language
- this refers to comments as well as blocks of logic
While I was probably not 100% enforcing all this points, the areas I reviewed I believe serve as a good example where most of our logic goes awry. Just because we have a verbose API doesn't mean our blocks have to be verbose 🚀.
This review is by no means a blocker, but I would find value if the points are discussed collectively in the public chat.
| parts := strings.SplitN(id.OpaqueId, "!", 2) | ||
| if len(parts) != 2 { | ||
| return &provider.ListStorageSpacesResponse{ | ||
| Status: status.NewInvalidArg(ctx, "space id must be separated by !"), | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
I'd refactor this to something with more semantic meaning. All we see here is "something has to be split in 2 using ! as a delimiter, and this will probably be used wherever we use the spaces API.
|
|
||
| spacesFromProviders := make([][]*provider.StorageSpace, len(providers)) | ||
| errors := make([]error, len(providers)) | ||
| var wg sync.WaitGroup |
There was a problem hiding this comment.
| var wg sync.WaitGroup |
|
|
||
| uniqueSpaces := map[string]*provider.StorageSpace{} | ||
| for i := range providers { | ||
| if errors[i] != nil { |
There was a problem hiding this comment.
why would an error condition mean skip the provider? Should we check the error type? Could it be a "mission critical" error and halt this operation? Maybe i'm overthinking 🦖
There was a problem hiding this comment.
when a provider retuns an unsupported error we should continue with the others. expect failure.
There was a problem hiding this comment.
If there's one only provider which matches the request, we won't raise any errors and return an empty slice. Maybe we can handle that case separately?
|
@labkode @ishank011 this is ready for review |
|
|
||
| uniqueSpaces := map[string]*provider.StorageSpace{} | ||
| for i := range providers { | ||
| if errors[i] != nil { |
There was a problem hiding this comment.
If there's one only provider which matches the request, we won't raise any errors and return an empty slice. Maybe we can handle that case separately?
| // TODO if the storage id is not set but node id is set we could poll all storage providers to check if the node is known there | ||
| // for now, say the reference is invalid |
There was a problem hiding this comment.
Not sure about this one 😅
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
address small review items Co-authored-by: Alex Unger <6905948+refs@users.noreply.github.com>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
I split #1776 into two PRs:
This is the first PR about spaces. It
/homeand/users, both will return the the same spacesProviderId, only theProviderPath, which we use in a hacky way to ignore all storage providers that start with a/, eg./public,/home, etc. We then query only the remaining ones, which have a storage id set and can be routed to when using theResourceIdof a relativeReference.ListStorageSpacesimplementation for the decomposedfsListStorageSpacesstubs for the other storage providers