pandaproxy: Invert error_code dependencies #4722
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tchaikov
reviewed
May 12, 2022
Signed-off-by: Ben Pope <ben@redpanda.com>
There are currently several projects that depend on `v::pandaproxy_common` due to the conversion from `error_code` to `error_condition`, `reply_error_code` There are two ways to map to an `error_condition` 1) Override `error_category::default_error_condition` 2) Override `error_category::equivalent` 1) Is what's current, but it requires a global `default_error_condition`, but `reply_error_code` is specific to pandaproxy, so the dependency works the wrong way. 2) Could work, but only for checking equivalence of specific `std::error_code` with a specific `reply_error_code`, not for directly converting it. To break the dependency, overload `make_error_condition` that takes a `std::error_code`, and maps from `kafka::error_code` to `reply_error_condition` as before. Further commits will move code to break the dependencies. Implement replay_error_category::equivalent in terns of the new `make_error_condition`. Signed-off-by: Ben Pope <ben@redpanda.com>
Move `make_error_code` and `error_category` from pandaproxy to kafka. Signed-off-by: Ben Pope <ben@redpanda.com>
Move it from the pandaproxy::parse to pandaproxy. Change `exception_base` to use an `error_code`, which is the correct tool for passing around error information. Signed-off-by: Ben Pope <ben@redpanda.com>
Move it from the pandaproxy::json to pandaproxy. Signed-off-by: Ben Pope <ben@redpanda.com>
BenPope
force-pushed
the
move-kafka-error-code-to-kafka
branch
from
3c044bc
to
fd0eeb4
Compare
BenPope
changed the title
Merged
dotnwat
approved these changes
May 16, 2022
| @@ -190,6 +191,17 @@ std::error_condition make_error_condition(std::error_code ec) { | |||
| return rec::kafka_authentication_error; | |||
| } | |||
| return {}; // keep gcc happy | |||
| } else if (ec.category() == make_error_code(pec::empty_param).category()) { | |||
| switch (static_cast<pec>(ec.value())) { | |||
| case pec::empty_param: | |||
There was a problem hiding this comment.
I tend to do that only for cases where there is a statement. I don't think the compiler will warn unless there is a statement, anyhow.
There was a problem hiding this comment.
hmm, i thought clang-tidy warned in this case, but indeed im not see it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cover letter
Refactor pandaproxy errors, so that the conversion of
error_codestoreply_error_conditionbecomes the responsibility of pandaproxy, not each submodule. This makes the dependencies the right way round.There are two ways to map to an
error_conditionerror_category::default_error_conditionerror_category::equivalentHowever:
default_error_condition, butreply_error_codeis specificto pandaproxy, so the dependency works the wrong way.
std::error_codewith a specificreply_error_code, notfor directly converting it.
Solution
To break the dependency, overload
make_error_conditionthattakes a
std::error_code, and maps fromkafka::error_codetoreply_error_conditionas before. Further commits will move codeto break the dependencies.
Implement
reply_error_category::equivalentin terns ofthe new
make_error_condition.error_categoryandmake_error_codefor kafka into kafkaerror_codetoerror_conditioncode from pandaproxy::parse to pandaproxyerror_codetoerror_conditioncode from pandaproxy::json to pandaproxyUseful resource: https://akrzemi1.wordpress.com/2017/09/04/using-error-codes-effectively/
Closes #4307 - this is a prerequisite of updating libfmt #4260
In a future PR, it will be possible to split the error_codes for Schema Registry and REST API.
Alternative
If
reply_error_category::equivalentwere to be implemented instead, it would something like:And then the other ~18
reply_error codeswould have to be implemented for all theerror_codesin other namespaces such aspandaproxy::parse,pandaproxy::json, etc., and there would be no way to have the compiler statically check for incomplete handling of enum cases in the distant modules. In addition, conversion to an http status code would have to exhaustive switch over eachreply_error_code., requiring two conversions.Signed-off-by: Ben Pope ben@redpanda.com
Release notes