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

kafka/error: define is_error_code_enum trait in error.h #4307

Closed
wants to merge 1 commit into from

Conversation

tchaikov
Copy link
Contributor

@tchaikov tchaikov commented Apr 17, 2022

move the is_error_code_enumkafka::error_code trait into error.h
where kafka::error_code is defined. this would help us ease the
pain of fight with the failures like:

FAILED: src/v/pandaproxy/CMakeFiles/v_pandaproxy_common.dir/server.cc.o
ccache /usr/bin/clang++-15 -DBOOST_ASIO_HAS_STD_INVOKE_RESULT -DFMT_LOCALE -DFMT_SHARED -DSEASTAR_API_LEVEL=6 -DSEASTAR_DEBUG -DSEASTAR_DEBUG_SHARED_PTR -DSEASTAR_DEFAULT_ALLOCATOR -DSEASTAR_SCHEDULING_GROUPS_COUNT=16 -DSEASTAR_SHUFFLE_TASK_QUEUE -DSEASTAR_TYPE_ERASE_MORE -DXXH_PRIVATE_API -DZSTD_STATIC_LINKING_ONLY -I/home/kefu/dev/redpanda/src/v -I/home/kefu/dev/redpanda/build/src/v -isystem /home/kefu/dev/redpanda/build/deps_install/include -isystem /home/kefu/dev/redpanda/build/deps_install/include/hdr -fPIC -fsanitize=undefined -fsanitize=address -fcolor-diagnostics -g -fPIC -Wall -Wextra -Werror -Wno-missing-field-initializers -std=c++20 -U_FORTIFY_SOURCE -DSEASTAR_SSTRING -Werror=unused-result "-Wno-error=#warnings" -fstack-clash-protection -fsanitize=address -fsanitize=undefined -fno-sanitize=vptr -std=c++20 -MD -MT src/v/pandaproxy/CMakeFiles/v_pandaproxy_common.dir/server.cc.o -MF src/v/pandaproxy/CMakeFiles/v_pandaproxy_common.dir/server.cc.o.d -o src/v/pandaproxy/CMakeFiles/v_pandaproxy_common.dir/server.cc.o -c /home/kefu/dev/redpanda/src/v/pandaproxy/server.cc
In file included from /home/kefu/dev/redpanda/src/v/pandaproxy/server.cc:17:
In file included from /home/kefu/dev/redpanda/src/v/pandaproxy/reply.h:16:
/home/kefu/dev/redpanda/src/v/pandaproxy/error.h:73:8: error: explicit specialization of 'std::is_error_code_enum<kafka::error_code>' after instantiation
struct is_error_code_enum<kafka::error_code> : true_type {};
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/system_error:206:17: note: implicit instantiation first required here
             enable_if<is_error_code_enum<_ErrorCodeEnum>::value>::type>
                       ^
In file included from /home/kefu/dev/redpanda/src/v/pandaproxy/server.cc:17:
/home/kefu/dev/redpanda/src/v/pandaproxy/reply.h:103:16: error: no matching function for call to 'errored_body'
        return errored_body(e.error, e.what());
               ^~~~~~~~~~~~
/home/kefu/dev/redpanda/src/v/pandaproxy/reply.h:70:1: note: candidate function not viable: no known conversion from 'const kafka::error_code' to 'std::error_condition' for 1st argument
errored_body(std::error_condition ec, ss::sstring msg) {
^
/home/kefu/dev/redpanda/src/v/pandaproxy/reply.h:80:1: note: candidate function not viable: no known conversion from 'const kafka::error_code' to 'std::error_code' for 1st argument
errored_body(std::error_code ec, ss::sstring msg) {
^
2 errors generated.

Signed-off-by: Kefu Chai tchaikov@gmail.com

Cover letter

Describe in plain language the motivation (bug, feature, etc.) behind the change in this PR and how the included commits address it.

Fixes #ISSUE-NUMBER, Fixes #ISSUE-NUMBER, ...

Release notes

@tchaikov
Copy link
Contributor Author

without this change, the tree just does not compile with clang-15. and i think it'd be a better practice to colocate the traits along with the "traited" type, hence the change.

@BenPope
Copy link
Member

BenPope commented Apr 20, 2022

Thanks @tchaikov, I'm following along with the changes you're proposing, but have very limited time until early next month.

I think it makes sense to move make_error_code(error_code) too.

@tchaikov
Copy link
Contributor Author

Thanks @tchaikov, I'm following along with the changes you're proposing, but have very limited time until early next month.

@BenPope hi Ben, sure. please take your time.

I think it makes sense to move make_error_code(error_code) too.

agreed! moved and repushed.

Copy link
Member

@BenPope BenPope left a comment

Choose a reason for hiding this comment

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

There's a compilation failure:

../../../src/v/kafka/protocol/errors.cc:206:35: error: use of undeclared identifier 'pandaproxy'
--
  | return {static_cast<int>(ec), pandaproxy::kafka_error_category};

Copy link
Member

@BenPope BenPope left a comment

Choose a reason for hiding this comment

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

There's a compilation failure:

../../../src/v/kafka/protocol/errors.cc:206:35: error: use of undeclared identifier 'pandaproxy'
--
  | return {static_cast<int>(ec), pandaproxy::kafka_error_category};

@tchaikov tchaikov force-pushed the wip-error-condition branch 3 times, most recently from 9f1dafa to cb39bcb Compare May 11, 2022 02:12
* move the is_error_code_enum<kafka::error_code> trait into error.h
  where kafka::error_code is defined. this would help us ease the
  pain of fighting with the failures attached at the end of the
  the commit message.
* move make_error_code() also into errors.{h,cc}.
  because as make_error_code(error_code) is also a part of the error
  handling facility built around error_code, it is "closer" to
  error_code. so let's put them together, for better readability and
  maintainabilty.
* in order to get access to
  `pandaproxy::make_error_condition(pandaproxy::reply_error_code)`,
  from `v::kafka_protocol`, the linkage to `v/pandaproxy/error.cc`
  is added to `v::kafka_protocol` via `v::pandaproxy_common`

----

FAILED: src/v/pandaproxy/CMakeFiles/v_pandaproxy_common.dir/server.cc.o
ccache /usr/bin/clang++-15 -DBOOST_ASIO_HAS_STD_INVOKE_RESULT -DFMT_LOCALE -DFMT_SHARED -DSEASTAR_API_LEVEL=6 -DSEASTAR_DEBUG -DSEASTAR_DEBUG_SHARED_PTR -DSEASTAR_DEFAULT_ALLOCATOR -DSEASTAR_SCHEDULING_GROUPS_COUNT=16 -DSEASTAR_SHUFFLE_TASK_QUEUE -DSEASTAR_TYPE_ERASE_MORE -DXXH_PRIVATE_API -DZSTD_STATIC_LINKING_ONLY -I/home/kefu/dev/redpanda/src/v -I/home/kefu/dev/redpanda/build/src/v -isystem /home/kefu/dev/redpanda/build/deps_install/include -isystem /home/kefu/dev/redpanda/build/deps_install/include/hdr -fPIC -fsanitize=undefined -fsanitize=address -fcolor-diagnostics -g -fPIC -Wall -Wextra -Werror -Wno-missing-field-initializers -std=c++20 -U_FORTIFY_SOURCE -DSEASTAR_SSTRING -Werror=unused-result "-Wno-error=#warnings" -fstack-clash-protection -fsanitize=address -fsanitize=undefined -fno-sanitize=vptr -std=c++20 -MD -MT src/v/pandaproxy/CMakeFiles/v_pandaproxy_common.dir/server.cc.o -MF src/v/pandaproxy/CMakeFiles/v_pandaproxy_common.dir/server.cc.o.d -o src/v/pandaproxy/CMakeFiles/v_pandaproxy_common.dir/server.cc.o -c /home/kefu/dev/redpanda/src/v/pandaproxy/server.cc
In file included from /home/kefu/dev/redpanda/src/v/pandaproxy/server.cc:17:
In file included from /home/kefu/dev/redpanda/src/v/pandaproxy/reply.h:16:
/home/kefu/dev/redpanda/src/v/pandaproxy/error.h:73:8: error: explicit specialization of 'std::is_error_code_enum<kafka::error_code>' after instantiation
struct is_error_code_enum<kafka::error_code> : true_type {};
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/system_error:206:17: note: implicit instantiation first required here
             enable_if<is_error_code_enum<_ErrorCodeEnum>::value>::type>
                       ^
In file included from /home/kefu/dev/redpanda/src/v/pandaproxy/server.cc:17:
/home/kefu/dev/redpanda/src/v/pandaproxy/reply.h:103:16: error: no matching function for call to 'errored_body'
        return errored_body(e.error, e.what());
               ^~~~~~~~~~~~
/home/kefu/dev/redpanda/src/v/pandaproxy/reply.h:70:1: note: candidate function not viable: no known conversion from 'const kafka::error_code' to 'std::error_condition' for 1st argument
errored_body(std::error_condition ec, ss::sstring msg) {
^
/home/kefu/dev/redpanda/src/v/pandaproxy/reply.h:80:1: note: candidate function not viable: no known conversion from 'const kafka::error_code' to 'std::error_code' for 1st argument
errored_body(std::error_code ec, ss::sstring msg) {
^
2 errors generated.

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
@BenPope
Copy link
Member

BenPope commented May 12, 2022

@tchaikov Thanks for this. Unfortunately this creates a dependency cycle between kafka and pandaproxy, which already exists in the other direction.

I did some hacking and this is about the best I came up with: #4722

@tchaikov
Copy link
Contributor Author

tchaikov commented May 12, 2022

@tchaikov Thanks for this. Unfortunately this creates a dependency cycle between kafka and pandaproxy, which already exists in the other direction.

I did some hacking and this is about the best I came up with: #4722

@BenPope hi Ben, thanks for investigating into this, i meant to post my concern on this dependency. not only it introduces a cyclic dependency but also it is a bad smell -- i felt that kafka should be a relatively low-level facility which is supposed to be a dependency of other CMake targets like pandaproxy.

i will close my PR once yours gets approved and merged.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants