MINIFICPP-2479 Update librdkafka and add lz4, zstd support #1885
Conversation
| install(EXPORT Lz4Targets | ||
| FILE Lz4Targets.cmake # Generate Lz4Targets.cmake file | ||
| NAMESPACE lz4:: # Set the namespace to lz4:: | ||
| DESTINATION lib/cmake/Lz4 # Where to install the CMake config | ||
| ) |
There was a problem hiding this comment.
I'm not sure about the install calls, they usually install files in the release tarball, and through that the user's machine. Are you sure these are not doing that?
There was a problem hiding this comment.
I'm not quite sure if this was the right approach, but without this librdkafka cmake calls fail due to
install(EXPORT "RdKafkaTargets" ...) includes target "rdkafka" which requires target "lz4_static" that is not in any export set.
martinzink
force-pushed
the
MINIFICPP-2479_glow_up_kafka
branch
from
85afcbd to
5b8a9e8
Compare
There was a problem hiding this comment.
There are a lot of if statements that went from multiline from single line, and some property builder calls that are also on a single long line now. I don't think that's necessarily a readability improvement, unless the statement is trivial.
If your IDE did it automatically, I suggest to turn off automatic shortening to a single line. If you did it manually after a conscious decision, then it's fine, I'm rarely criticizing formatting style choices, but I would be less aggressive with putting too much logic on a single line.
extensions/kafka/PublishKafka.cpp
Outdated
| const gsl::owner<rd_kafka_headers_t*> result{ rd_kafka_headers_new(8) }; | ||
| if (!result) { throw std::bad_alloc{}; } |
There was a problem hiding this comment.
the null check is missing in the new version
There was a problem hiding this comment.
I've readded it in readd nullcheck after malloc
although I don't think we check every malloc/new in the codebase, but it couldnt hurt either
I've utilized the new .clang-format to reformat the extension, I agree that its a bit aggressive so I've refined the clang-format so we don't target the linter's 200 long max line length, but rather a more reasonable 125. |
|
I thought "AllowShortIfStatementsOnASingleLine" means allow, so don't reformat single line ones where they see them, but reformat otherwise. |
extensions/kafka/ConsumeKafka.h
Outdated
| "Specifies whether or not MiNiFi should honor transactional guarantees when communicating with Kafka. If " | ||
| "false, the Processor will use " | ||
| "an \"isolation level\" of " | ||
| "read_uncomitted. This means that messages will be received as soon as they are written to Kafka but will be " | ||
| "pulled, even if the " | ||
| "producer cancels the transactions. " | ||
| "If this value is true, MiNiFi will not receive any messages for which the producer's transaction was " | ||
| "canceled, but this can result in " | ||
| "some latency since the consumer " | ||
| "must wait for the producer to finish its entire transaction instead of pulling as the messages become " | ||
| "available.") |
There was a problem hiding this comment.
This clang-format reflow resulted in some unnecessary shorter lines. Please check the rest of the file too, there are more of these.
extensions/kafka/PublishKafka.cpp
Outdated
| result = | ||
| rd_kafka_conf_set(conf_.get(), "message.max.bytes", max_message_size->c_str(), err_chars.data(), err_chars.size()); |
There was a problem hiding this comment.
I don't like this formatting change either :/
martinzink
force-pushed
the
MINIFICPP-2479_glow_up_kafka
branch
from
3362d62 to
3fd27e0
Compare
|
The failures look like a rebase issue |
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?
For code changes:
For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.