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

[pulsar-client-go]add producer.LastSequenceID and message SequenceID in Go client #3416

Merged
merged 6 commits into from
Jan 25, 2019
Merged

Conversation

wolfstudy
Copy link
Member

@wolfstudy wolfstudy commented Jan 24, 2019

Signed-off-by: xiaolong.ran ranxiaolong716@gmail.com

Motivation

Add producer.LastSequenceID and message SequenceID in Go client.

Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>
@@ -163,6 +163,14 @@ type Producer interface {
// the eventual error in publishing
SendAsync(context.Context, ProducerMessage, func(ProducerMessage, error))

// Get the last sequence id that was published by this producer.
// This represent either the automatically assigned or custom sequence id (set on the MessageBuilder) that
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of MessageBuilder which is in Java client lib, we should refer to ProducerMessage here (https://github.com/apache/pulsar/blob/master/pulsar-client-go/pulsar/message.go#L24)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the ProducerMessage doesn't currently have a way to specify the sequence id of a particular message. This should be done by using pulsar_message_set_sequence_id() function.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, you are reight. I will provide this interface to achieve the corresponding function in next pull request.

pulsar-client-go/pulsar/producer.go Outdated Show resolved Hide resolved
@merlimat merlimat added type/feature The PR added a new feature or issue requested a new feature component/go labels Jan 24, 2019
@merlimat merlimat added this to the 2.3.0 milestone Jan 24, 2019
Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>
Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>
Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>
Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>
@wolfstudy
Copy link
Member Author

PTAL @merlimat thanks

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.

LGTM, juts one last comment on renaming ID -> SequenceID

@@ -36,6 +36,9 @@ type ProducerMessage struct {

// Override the replication clusters for this message.
ReplicationClusters []string

// Set the sequence id to assign to the current message
ID int64
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use SequenceID here to avoid confusion with the "MessageId" concept which is different. eg. On a received message, Message.ID() will give you the MessageId (assigned by brokers) rather than the sequence id (assigned by producer).

Copy link
Contributor

Choose a reason for hiding this comment

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

At the same time, I think we should also expose a way to retrieve the sequence id on a received message as in Message.SequenceID() method.

Copy link
Member Author

@wolfstudy wolfstudy Jan 25, 2019

Choose a reason for hiding this comment

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

yes, SequenceID sounds good. About Message.SequenceID() method, i will provide in the next pull request.

pulsar-client-go/pulsar/producer.go Outdated Show resolved Hide resolved
Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>
@wolfstudy wolfstudy changed the title [pulsar-client-go]add producer.GetLastSequenceID in Go client [pulsar-client-go]add producer.LastSequenceID and message SequenceID in Go client Jan 25, 2019
@wolfstudy
Copy link
Member Author

@merlimat PTAL again

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.

👍

@merlimat
Copy link
Contributor

run java8 tests
run integration tests

@merlimat merlimat merged commit 8eb4ae2 into apache:master Jan 25, 2019
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.

None yet

2 participants