Skip to content

Conversation

@mfontanini
Copy link
Owner

Relates to #128

@mfontanini
Copy link
Owner Author

@accelerated can you look into this one? I don't think we need that constructor, given the only one that constructs MessageTimestamps is cppkafka itself.

@accelerated
Copy link
Contributor

accelerated commented Oct 26, 2018

See my comment in the Issue thread. I can move it to the header file, that was my oversight. I can also remove it, but I did make a note about that when I submitted the PR. Imho it's best to overload the MessageBuilder with a MessageTimestamp as argument to timeout().

@mfontanini
Copy link
Owner Author

I don't think a user should be able to create MessageTimestamp (right now it can't). The user shouldn't be able to specify a MessageTimestamp::TimestampType because when calling rd_kafka_producev that goes away: at that point it's just a millisecond count.

Given that nothing constructs MessageTimestamp besides the Message, there's no point in allowing creating them from time_point

@accelerated
Copy link
Contributor

Ok, looks good to me! Merge away :)

@accelerated
Copy link
Contributor

PS: A few minor observations:

  • The MessageTimestamp class should ideally be in its own header.
  • Its constructor should be private and Message class should be a friend. Then there's no confusion as to weather or not you can instantiate this class.

@mfontanini mfontanini merged commit 451d602 into master Oct 27, 2018
@mfontanini mfontanini deleted the time-point-fixes branch October 27, 2018 01:57
@mfontanini
Copy link
Owner Author

Yep, those are good ideas.

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