Skip to content

Added time_point overloads for creating timestamps. #128

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

Merged
merged 2 commits into from
Oct 25, 2018
Merged

Added time_point overloads for creating timestamps. #128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60f07a6
Added time_point overloads for creating timestamps.
Oct 25, 2018
2430624
aliased std::chrono types
Oct 25, 2018
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
11 changes: 9 additions & 2 deletions include/cppkafka/message.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,12 +240,19 @@ class CPPKAFKA_API MessageTimestamp {
};

/**
* Constructs a timestamp object
* Constructs a timestamp object using a 'duration'.
*/
MessageTimestamp(std::chrono::milliseconds timestamp, TimestampType type);

/**
* Constructs a timestamp object using a 'time_point'.
*/
template <typename Clock, typename Duration = typename Clock::duration>
MessageTimestamp(std::chrono::time_point<Clock, Duration> timestamp, TimestampType type);
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not clear to me why it makes sense to have this template constructor be public, since its definition is only available in the .cpp file, and there's no explicit instantiation of it. In fact, I don't see any instantiations of this constructor

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, the definition should be on the header. It would never work the way it is.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, true, why is this constructor even here? Nothing uses it and nothing can construct a timestamp besides cppkafka itself.

Copy link
Owner

Choose a reason for hiding this comment

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

I put up #129 to fix this. I don't think we need that overload at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can move the def from the .cpp file. I initially did it w/o templates, and then after you said it was ok to templetize based on the clock type I just added the templates but forgot to move it to the header file. My bad.


/**
* Gets the timestamp value
* Gets the timestamp value. If the timestamp was created with a 'time_point',
* the duration represents the number of milliseconds since epoch.
*/
std::chrono::milliseconds get_timestamp() const;

Expand Down
21 changes: 19 additions & 2 deletions include/cppkafka/message_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,19 @@ class BasicMessageBuilder {
Concrete& payload(BufferType&& value);

/**
* Sets the message's timestamp
* Sets the message's timestamp with a 'duration'
*
* \param value The timestamp to be used
*/
Concrete& timestamp(std::chrono::milliseconds value);

/**
* Sets the message's timestamp with a 'time_point'.
*
* \param value The timestamp to be used
*/
template <typename Clock, typename Duration = typename Clock::duration>
Concrete& timestamp(std::chrono::time_point<Clock, Duration> value);

/**
* Sets the message's user data pointer
Expand Down Expand Up @@ -184,7 +192,8 @@ class BasicMessageBuilder {
BufferType& payload();

/**
* Gets the message's timestamp
* Gets the message's timestamp as a duration. If the timestamp was created with a 'time_point',
* the duration represents the number of milliseconds since epoch.
*/
std::chrono::milliseconds timestamp() const;

Expand Down Expand Up @@ -295,6 +304,14 @@ C& BasicMessageBuilder<T, C>::timestamp(std::chrono::milliseconds value) {
return get_concrete();
}

template <typename T, typename C>
template <typename Clock, typename Duration>
C& BasicMessageBuilder<T, C>::timestamp(std::chrono::time_point<Clock, Duration> value)
{
timestamp_ = std::chrono::duration_cast<std::chrono::milliseconds>(value.time_since_epoch());
return get_concrete();
}

template <typename T, typename C>
C& BasicMessageBuilder<T, C>::user_data(void* value) {
user_data_ = value;
Expand Down
10 changes: 9 additions & 1 deletion src/message.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,18 @@ Message& Message::load_internal() {
// MessageTimestamp

MessageTimestamp::MessageTimestamp(milliseconds timestamp, TimestampType type)
: timestamp_(timestamp), type_(type) {
: timestamp_(timestamp),
type_(type) {

}

template <typename Clock, typename Duration>
MessageTimestamp::MessageTimestamp(std::chrono::time_point<Clock, Duration> timestamp, TimestampType type)
: timestamp_(std::chrono::duration_cast<std::chrono::milliseconds>(timestamp.time_since_epoch())),
type_(type) {

}

milliseconds MessageTimestamp::get_timestamp() const {
return timestamp_;
}
Expand Down
6 changes: 4 additions & 2 deletions tests/producer_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ using std::condition_variable;
using std::chrono::system_clock;
using std::chrono::seconds;
using std::chrono::milliseconds;
using std::chrono::time_point;
using std::chrono::duration_cast;
using std::ref;

using namespace cppkafka;
Expand Down Expand Up @@ -164,7 +166,7 @@ TEST_CASE("simple production", "[producer]") {
SECTION("message with key") {
const string payload = "Hello world! 2";
const string key = "such key";
const milliseconds timestamp{15};
auto timestamp = system_clock::now();
Producer producer(config);
producer.produce(MessageBuilder(KAFKA_TOPICS[0]).partition(partition)
.key(key)
Expand All @@ -181,7 +183,7 @@ TEST_CASE("simple production", "[producer]") {
CHECK(message.get_partition() == partition);
CHECK(!!message.get_error() == false);
REQUIRE(!!message.get_timestamp() == true);
CHECK(message.get_timestamp()->get_timestamp() == timestamp);
CHECK(message.get_timestamp()->get_timestamp() == duration_cast<milliseconds>(timestamp.time_since_epoch()));
Copy link
Contributor

Choose a reason for hiding this comment

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

this test does not exercise the new constructor. If it did, you would notice that the definition is not available, and will produce a link error

}

#if (RD_KAFKA_VERSION >= RD_KAFKA_HEADERS_SUPPORT_VERSION)
Expand Down