Skip to content

Update the KafkaException using modern practices #241

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.

Sign up for GitHub

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

Open
downfa11 wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Update the KafkaException using modern practices #241

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d117f9c
Refactor kafkaException constructor
downfa11 Sep 13, 2024
b0793aa
Refactor kafkaException constructor
downfa11 Sep 13, 2024
f39a4fb
Merge branch 'main' of https://github.com/downfa11/modern-cpp-kafka
downfa11 Aug 17, 2025
0e3a826
refactor: Replace exception macros with source_location
downfa11 Aug 17, 2025
dfd6121
Removed KafkaException dependencies
downfa11 Aug 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions include/kafka/KafkaException.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <chrono>
#include <exception>
#include <string>
#include <source_location>


namespace KAFKA_API {
Expand All @@ -21,10 +22,10 @@ namespace KAFKA_API {
class KafkaException: public std::exception
{
public:
KafkaException(const char* filename, std::size_t lineno, const Error& error)
KafkaException(const std::source_location location = std::source_location::current(), const Error& error)
Copy link
Contributor

@kenneth-jia kenneth-jia Aug 14, 2025

Choose a reason for hiding this comment

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

  • the std::source_location is quit an elegant choice, but it's not supported until c++20. (the project has to be compatible with c++14/17)
  • default parameter values must be specified starting from the rightmost parameter and moving leftwards
  • if we're using std::source_location as the default parameter, we could get rid of the macro KAFKA THROW_ERROR as well -- just throw KafkaException(error) at where we need it.

Copy link
Author

@downfa11 downfa11 Aug 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the feedback!

As far as I know, source_location was originally provided in Boost before being adopted into C++20.

Since project already depends on Boost (e.g., for boost::optional), using boost::source_location will not introduce any new external dependencies.

I also plan to update the code so that exceptions can be thrown directly via KafkaException(error) and remove the KAFKA_THROW_ERROR macro in the process.

What do you think?

Copy link
Contributor

@kenneth-jia kenneth-jia Aug 15, 2025

Choose a reason for hiding this comment

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

Yes, we have to use some trick for the optional, -- https://github.com/morganstanley/modern-cpp-kafka/blob/main/include/kafka/Types.h#L19
It also make the dependencies messy, https://github.com/morganstanley/modern-cpp-kafka/blob/main/.github/workflows/kafka_api_ci_tests.yml#L307

Copy link
Author

@downfa11 downfa11 Aug 17, 2025

Choose a reason for hiding this comment

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

While the boost::source_location solution is certainly elegant, I'm wondering if introducing a new Boost dependency might be a concern for the project.

I'd like to make sure we aren't adding complexity where we're trying to remove it.

: _when(std::chrono::system_clock::now()),
_filename(filename),
_lineno(lineno),
_filename(location.file_name()),
_lineno(location.line()),
_error(std::make_shared<Error>(error))
{}

Expand Down Expand Up @@ -53,7 +54,7 @@ class KafkaException: public std::exception
};


#define KAFKA_THROW_ERROR(error) throw KafkaException(__FILE__, __LINE__, error)
#define KAFKA_THROW_ERROR(error) throw KafkaException(error)
#define KAFKA_THROW_IF_WITH_ERROR(error) if (error) KAFKA_THROW_ERROR(error)

} // end of KAFKA_API
Expand Down