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

Added time_point overloads for creating timestamps. #128

Merged
merged 2 commits into from
Oct 25, 2018

Conversation

accelerated
Copy link
Contributor

@accelerated accelerated commented Oct 25, 2018

Changes to BasicMessageBuilder and MessageTimestamp classes to allow std::chrono::time_point overloads.

Note that MessageTimestamp is created internally by the Message object, however since it's constructor was public, I figured others could use this class to store timestamps outside of the Message implementation.

Fixes #15

@accelerated
Copy link
Contributor Author

Note2 we could get rid of the template parameters and just force the time point to be std::chrono::time_point<std::chrono::system_clock, std::chrono::milliseconds>. Not sure if it's a good idea though.

@mfontanini
Copy link
Owner

Thanks for the PR. Regarding your system clock comment I was thinking about that and I have my doubts but I think letting the user provide a time point from any clock makes sense. I don't know why someone would want a time point from a steady clock but if that's what they want, then let them be.

@accelerated
Copy link
Contributor Author

accelerated commented Oct 25, 2018

They could use high_precision_clock also...however having the template params allows you to do .timestamp(std::chrono::system_clock::now()) without any casts which is nice.

@mfontanini
Copy link
Owner

Yeah, I was using steady clock as an example of a clock that makes no sense out of the computer it was generated on, but any other will do.

@mfontanini mfontanini merged commit 57268e6 into mfontanini:master Oct 25, 2018
@@ -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

* 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.

@accelerated
Copy link
Contributor Author

accelerated commented Oct 26, 2018

@mfontanini see my note in the first comment I made. The idea was that since the other constructor is public (should have been private) and Message class should have been made friend class, perhaps others, including myself could use the MessageTimestamp class as an object "outside" the Message class, to store timestamps, etc. It doens't have to be a one-to-one relationship.
This being said, the BasicMessageBuilder class could take MessageTimestamp as an argument to timestamp() method instead of the two chrono types and MessageTimestamp can be constructed using both. This would be a cleaner API imho.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants