morganstanley / modern-cpp-kafka

A C++ API for Kafka clients (i.e. KafkaProducer, KafkaConsumer, AdminClient)
Apache License 2.0
331 stars 86 forks source link

Deque poll argument #204

Closed KiillThemAll closed 1 year ago

KiillThemAll commented 1 year ago

Im using poll results in internal queue -> added std::deque& arg variant to poll function

KiillThemAll commented 1 year ago

Im just curious how to pass all builds with -warnings-as-errors...

kenneth-jia commented 1 year ago

Hi, @KiillThemAll Maybe you don't really need thepollMessages(int timeoutMs, std::deque<consumer::ConsumerRecord>& output) interface. How about using the follwing instead?

std::vector<consumer::ConsumerRecord> recordsInVector = theCosumer.pollMessages(int timeoutMs);
std::deque<consumer::ConsumerRecord> recordsInDeque;
std::move(recordsInVector.begin(), recordsInVector.end(), recordsInDeque.begin());
KiillThemAll commented 1 year ago

According to https://en.cppreference.com/w/cpp/algorithm/move there will be additional complexity on moving vector to deque: last - first move assignments. image

kenneth-jia commented 1 year ago

According to https://en.cppreference.com/w/cpp/algorithm/move there will be additional complexity on moving vector to deque: last - first move assignments. image Exactly, the "Complixity" is last-first move assignments, -- so is the messages processing. If you're interested, could try profiling your app, -- here's not the place could be optimized.

KiillThemAll commented 1 year ago

There will be no std::move(recordsInVector.begin(), recordsInVector.end(), recordsInDeque.begin()); at all if we pass deque by reference. image

kenneth-jia commented 1 year ago

There will be no std::move(recordsInVector.begin(), recordsInVector.end(), recordsInDeque.begin()); at all if we pass deque by reference.)

Hi, @KiillThemAll I don't think it's a good idea to expose too many interfaces for people, -- there's already a std::vector<consumer::ConsumerRecord> poll(std::chrono::milliseconds), and that should be enough. The poll(std::chrono::milliseconds timeout, std::vector<ConsumerRecord>& output) should not be there, nor the poll(std::chrono::milliseconds timeout, std::deque<ConsumerRecord>& output),

So, in some later day, I'll submit a PR to remove poll(std::chrono::milliseconds timeout, std::vector<ConsumerRecord>& output)...

kenneth-jia commented 1 year ago

So, in some later day, I'll submit a PR to remove poll(std::chrono::milliseconds timeout, std::vector<ConsumerRecord>& output)...

The interface has been removed with https://github.com/morganstanley/modern-cpp-kafka/pull/205