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

Issue #735: Event Time support in c++ client #736

Merged
merged 3 commits into from
Sep 14, 2017

Conversation

sijie
Copy link
Member

@sijie sijie commented Aug 31, 2017

This change is part of #732 - adding event time setter and getter in both c++ and python client.

@@ -71,6 +71,11 @@ class MessageBuilder {
MessageBuilder& setPartitionKey(const std::string& partitionKey);

/**
* Set the event timestamp for the message.
*/
MessageBuilder& setEventTimestamp(uint64_t event_timestamp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: in C++ code we've been using camelCase for classes and var names, like eventTimestamp

@merlimat merlimat added the type/feature The PR added a new feature or issue requested a new feature label Aug 31, 2017
@merlimat merlimat added this to the 1.20.0-incubating milestone Aug 31, 2017
@sijie
Copy link
Member Author

sijie commented Aug 31, 2017

@merlimat cool. I was following the protobuf code convention. I just changed event_timestamp to eventTimestamp.

Copy link
Contributor

@jai1 jai1 left a comment

Choose a reason for hiding this comment

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

Remove the java changes - Rest is LGTM.

@@ -73,9 +73,26 @@
*/
MessageId getMessageId();

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Java changes in C++ Client PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternately just merge the JAva PR and the difference won't be seen in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jai1 this change is based on #734. and they are organized in two different commits. once you merge #734 , this change will be only c++ changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, but I think you will still need to rebase this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

rebased.

sijie added 2 commits August 31, 2017 13:07
This change is adding `event-time` support in c++ and python client.
It is based on the change of apache#733
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

@jai1
Copy link
Contributor

jai1 commented Sep 2, 2017

retest this please

@merlimat merlimat merged commit 8f15858 into apache:master Sep 14, 2017
@merlimat merlimat mentioned this pull request Sep 14, 2017
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants