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

Folder restructure #178

Merged
merged 2 commits into from
Feb 22, 2023
Merged

Folder restructure #178

merged 2 commits into from
Feb 22, 2023

Conversation

glimchb
Copy link
Member

@glimchb glimchb commented Feb 21, 2023

split to several packages:

  • frontend
  • middleend
  • backend

This will help importing this library from other projects

TODOs:

  • remove some code dup in tests
  • split to more files like nvme vs virtio

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

split to several packages:
- frontend
- middleend
- backend

This will help importing this library from other projects

TODOs:
- remove some code dup in tests
- split to more files like nvme vs virtio

Signed-off-by: Boris Glimcher <[email protected]>
@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Merging #178 (9b9f4e6) into main (a15fb43) will decrease coverage by 4.96%.
The diff coverage is 40.27%.

@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
- Coverage   48.72%   43.76%   -4.97%     
==========================================
  Files           4        4              
  Lines        1250     1250              
==========================================
- Hits          609      547      -62     
- Misses        622      688      +66     
+ Partials       19       15       -4     
Impacted Files Coverage Δ
pkg/server/jsonrpc.go 0.00% <0.00%> (-81.58%) ⬇️
pkg/backend/backend.go 25.41% <21.56%> (ø)
pkg/frontend/frontend.go 54.95% <50.84%> (ø)
pkg/middleend/middleend.go 58.57% <58.62%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Boris Glimcher <[email protected]>
@glimchb glimchb requested a review from intelfisz February 21, 2023 16:19
@glimchb glimchb marked this pull request as ready for review February 21, 2023 16:19
@glimchb glimchb requested a review from a team as a code owner February 21, 2023 16:19
@glimchb glimchb requested a review from artek-koltun February 21, 2023 16:42
@glimchb glimchb added the Merge Candidate in the open merge window, next candidate for merge label Feb 21, 2023
}

log.Printf("SPDK mockup Server: got : %s", string(data))
log.Printf("SPDK mockup Server: snd : %s", spdk)
Copy link
Contributor

Choose a reason for hiding this comment

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

sEnd?

pb.RegisterNullDebugServiceServer(s, &server.Server{})
pb.RegisterAioControllerServiceServer(s, &server.Server{})
pb.RegisterMiddleendServiceServer(s, &server.Server{})
pb.RegisterFrontendNvmeServiceServer(s, &frontend.Server{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be sufficient to create only one frontend.Server instance and pass it into RegisterFrontendNvmeServiceServer, RegisterFrontendVirtioBlkServiceServer etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm good question

Copy link
Contributor

@sandersms sandersms left a comment

Choose a reason for hiding this comment

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

Changes look good. Other comments will be resolved at later time.

"testing"

"google.golang.org/protobuf/proto"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, but I'd organize this with the other protobuf import below

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll run goimport tool to auto-sort this

@glimchb
Copy link
Member Author

glimchb commented Feb 22, 2023

@artek-koltun I will merge this one and address your comments in a new PR that I already have pending.
Just want to sure I can import correctly this split from other repos and if not - fix it together

@glimchb glimchb merged commit 88edfc9 into opiproject:main Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge Candidate in the open merge window, next candidate for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants