Skip to content

Conversation

Forsworns
Copy link
Contributor

Describe what this PR does / why we need it

#450

Does this pull request fix one issue?

#450

Describe how you did it

The implementation is based on Kratos unified middleware abstractions for http/grpc servers/clients.

Tests on HTTP servers/clients are provided in server_test.go and client_test.go .

Describe how to verify it

The implementation can be verified via go test.

Special notes for reviews

@sczyh30 sczyh30 added area/integrations Issue related to integrations with open-source components to-review PRs to review labels Feb 11, 2022
@Forsworns Forsworns changed the title Kratos Add adapter for Kratos Feb 11, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2022

Codecov Report

Merging #459 (9a929d9) into master (89ef54d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #459   +/-   ##
=======================================
  Coverage   53.14%   53.14%           
=======================================
  Files          91       91           
  Lines        5897     5897           
=======================================
  Hits         3134     3134           
  Misses       2415     2415           
  Partials      348      348           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89ef54d...9a929d9. Read the comment docs.

@Casper-Mars
Copy link
Contributor

Why fallback only return error without response?

@Forsworns
Copy link
Contributor Author

Why fallback only return error without response?

  • I found all other adapters in Sentinel only return an error in their fallback options. I don't know if a response return value would be useful. Of course, it is more flexible :)
  • I found reply and error are mutually exclusive in some Kratos examples. Thus, I assume that the reply is nil when blockFallback is called in Sentinel middleware.
  • If necessary, the kratos/errors contains status code, reason and message fields. Informative responses can be appended in the message field.

@Casper-Mars
Copy link
Contributor

Casper-Mars commented Feb 11, 2022

Why fallback only return error without response?

  • I found all other adapters in Sentinel only return an error in their fallback options. I don't know if a response return value would be useful. Of course, it is more flexible :)
  • I found reply and error are mutually exclusive in some Kratos examples. Thus, I assume that the reply is nil when blockFallback is called in Sentinel middleware.
  • If necessary, the kratos/errors contains status code, reason and message fields. Informative responses can be appended in the message field.

Hi~Thanks for reply. The reason that reply is nil in kratos middleware is because those middleware doesnt invoke business logic. So they only need to report error.
But in practice, the fallback would invoke some business logic. Such as return previous value.
And I dont think return a reply within an error is a good idea. This require client to process special error with same business logic. At the same time, it couple results and errors. It make me confused that some other adapters only return an error.

@Forsworns
Copy link
Contributor Author

Why fallback only return error without response?

  • I found all other adapters in Sentinel only return an error in their fallback options. I don't know if a response return value would be useful. Of course, it is more flexible :)
  • I found reply and error are mutually exclusive in some Kratos examples. Thus, I assume that the reply is nil when blockFallback is called in Sentinel middleware.
  • If necessary, the kratos/errors contains status code, reason and message fields. Informative responses can be appended in the message field.

Hi~Thanks for reply. The reason that reply is nil in kratos middleware is because those middleware doesnt invoke business logic. So they only need to report error. But in practice, the fallback would invoke some business logic. Such as return previous value. And I dont think return a reply within an error is a good idea. This require client to process special error with same business logic. At the same time, it couple results and errors. It make me confused that some other adapters only return an error.

Thanks, got it, I'll add the reply to this adapter :) For other adapters, maybe a new issue should be open.

Copy link
Collaborator

@binbin0325 binbin0325 left a comment

Choose a reason for hiding this comment

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

LGTM

@shenqidebaozi
Copy link

You can consider submitting directly to Kratos contrib.

@Forsworns
Copy link
Contributor Author

You can consider submitting directly to Kratos contrib.

👍 I'll test it on the latest Kratos in these days and try to submit there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/integrations Issue related to integrations with open-source components to-review PRs to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants