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

refactor: transtition to spdk client #875

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

Conversation

artek-koltun
Copy link
Contributor

@artek-koltun artek-koltun commented Jan 29, 2024

spdk rolled out its own client, and we can start transition to that client from our custom solution

This PR is a first step of the integration.

An adapter struct between the new and old APIs was created in order to address the following obstacles:

  • Awkward response unmarshaling in the new client (SPDK team is working on that).
  • The new error formats are affecting a lot of tests. The adapter convert the new format to the old one. It enables a gradual transition. The tests will be reworked to the new errors, eliminating transformations in the adapter one by one.

Opens:

  • How to resist against error changes in spdk, so that we don't need to rework tons of tests?
    • Probability is low that the error will change frequently in SPDK. Do we really need to focus on that issue, then?
    • Use predefined set of errors and return them.
    • Check for keywords in an error message, not exact matching.
    • Check that error was received and disregard the content of the string.
    • ...
  • tracing is on our side or try to push an interface to spdk client (it knows all attributes) for tracing, so anyone could inject its own tracing functionality

Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

Attention: 66 lines in your changes are missing coverage. Please review.

Comparison is base (55e38cf) 74.77% compared to head (9d0d950) 74.19%.
Report is 6 commits behind head on main.

Files Patch % Lines
pkg/utils/server.go 0.00% 54 Missing ⚠️
pkg/spdk/adapter.go 86.04% 6 Missing ⚠️
pkg/backend/backend.go 50.00% 1 Missing and 1 partial ⚠️
pkg/frontend/frontend.go 71.42% 1 Missing and 1 partial ⚠️
pkg/middleend/middleend.go 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #875      +/-   ##
==========================================
- Coverage   74.77%   74.19%   -0.59%     
==========================================
  Files          40       41       +1     
  Lines        3699     3778      +79     
==========================================
+ Hits         2766     2803      +37     
- Misses        845      887      +42     
  Partials       88       88              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
type name will be used as spdk.SpdkClientAdapter by other
packages, and that stutters; consider calling this ClientAdapter

Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant