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

feat(backend): replace maps with gokv.Store abstraction #671

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

glimchb
Copy link
Member

@glimchb glimchb commented Sep 27, 2023

Signed-off-by: Boris Glimcher [email protected]

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #671 (4ba64c7) into main (bd7b947) will decrease coverage by 2.28%.
The diff coverage is 40.51%.

❗ Current head 4ba64c7 differs from pull request most recent head 51595e9. Consider uploading reports for the commit 51595e9 to get more accurate results

@@            Coverage Diff             @@
##             main     #671      +/-   ##
==========================================
- Coverage   76.53%   74.26%   -2.28%     
==========================================
  Files          35       34       -1     
  Lines        3533     4010     +477     
==========================================
+ Hits         2704     2978     +274     
- Misses        752      921     +169     
- Partials       77      111      +34     
Files Coverage Δ
pkg/backend/backend.go 62.50% <100.00%> (-10.23%) ⬇️
pkg/backend/aio.go 86.05% <41.81%> (-10.80%) ⬇️
pkg/backend/null.go 86.05% <41.81%> (-10.80%) ⬇️
pkg/backend/nvme_controller.go 59.64% <34.00%> (-17.08%) ⬇️
pkg/backend/nvme_path.go 64.49% <41.42%> (-5.60%) ⬇️

... and 8 files with indirect coverage changes

pkg/backend/aio.go Fixed Show fixed Hide fixed
pkg/backend/aio.go Fixed Show fixed Hide fixed
pkg/backend/aio.go Fixed Show fixed Hide fixed
@glimchb glimchb changed the title feat(backend): replace Aio map with gokv.Store abstraction feat(backend): replace maps with gokv.Store abstraction Sep 27, 2023
pkg/backend/null.go Fixed Show fixed Hide fixed
pkg/backend/null.go Fixed Show fixed Hide fixed
pkg/backend/null.go Fixed Show fixed Hide fixed
@glimchb glimchb force-pushed the aio branch 3 times, most recently from 80f0683 to 7c305ee Compare September 27, 2023 17:54
@glimchb glimchb added the help wanted Extra attention is needed label Sep 27, 2023
@glimchb glimchb force-pushed the aio branch 2 times, most recently from d2ba72a to 4ba64c7 Compare September 28, 2023 12:17
Copy link
Contributor

@artek-koltun artek-koltun left a comment

Choose a reason for hiding this comment

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

Looks good!
However, we need to try to implement keys collection and retrieval. Probably, by means of the idea we discussed last week. It will define if that storage library fits our needs

volume := new(pb.AioVolume)
found, err := s.store.Get(in.AioVolume.Name, volume)
if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd explicitly set gRPC error code as Internal in all storage related operations by means of status.Errof(codes.Internal,...)

@@ -43,14 +44,21 @@ func (s *Server) CreateNvmeRemoteController(_ context.Context, in *pb.CreateNvme
}
in.NvmeRemoteController.Name = utils.ResourceIDToVolumeName(resourceID)
// idempotent API when called with same key, should return same object
volume, ok := s.Volumes.NvmeControllers[in.NvmeRemoteController.Name]
if ok {
volume := new(pb.NvmeRemoteController)
Copy link
Contributor

Choose a reason for hiding this comment

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

controller := ...

@@ -161,8 +183,12 @@ func (s *Server) UpdateNvmePath(_ context.Context, in *pb.UpdateNvmePathRequest)
return nil, err
}
// fetch object from the database
volume, ok := s.Volumes.NvmePaths[in.NvmePath.Name]
if !ok {
volume := new(pb.NvmePath)
Copy link
Contributor

Choose a reason for hiding this comment

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

nvmePath := ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants