Skip to content

Comments

[Issue-148][pulsar-client-go] add seek by messageID#168

Merged
wolfstudy merged 5 commits intoapache:masterfrom
wangjia007bond:148
Feb 3, 2020
Merged

[Issue-148][pulsar-client-go] add seek by messageID#168
wolfstudy merged 5 commits intoapache:masterfrom
wangjia007bond:148

Conversation

@stevenwangnarvar
Copy link
Contributor

Master Issue: #148

Motivation

Add seek by messageID

Modifications

Add seek by messageID
Refactor some UT code

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for seek by messageID

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / GoDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@wolfstudy
Copy link
Member

@stevenwangnarvar Is it possible to split this pull request?

  • One of them is to implement seek by message ID
  • One of them is to refactoring the consumer_test.go

Copy link
Member

@wolfstudy wolfstudy left a comment

Choose a reason for hiding this comment

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

The check of golanglint-ci fail, please fix them.

you can run golangci-lint run -c ./golangci.yml ./... in locally.

pulsar/test_helper.go:201:1: context.Context should be the first parameter of a function (golint)
func send(t *testing.T, producer Producer, ctx context.Context, times int, message *ProducerMessage) {
^
pulsar/test_helper.go:216:1: context.Context should be the first parameter of a function (golint)
func receive(t *testing.T, consumer Consumer, ctx context.Context, times int, inspect func(*testing.T, Message, int)) {
^
pulsar/test_helper.go:25: File is not `goimports`-ed (goimports)
	"github.com/stretchr/testify/assert"
pulsar/consumer_test.go:56: File is not `goimports`-ed (goimports)
	msgProperties := map[string]string{ "key-1": "pulsar-1" }

func (c *consumer) Seek(msgID MessageID) error {
mid, ok := c.messageID(msgID)
if !ok {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we return an error here?

Choose a reason for hiding this comment

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

I checked other interfaces like Ack and NackID, they didn't handle error, because their interface didn't need return error, but I am not sure the correct behavior here. so I follow the current way.

seek by messageID
@wolfstudy
Copy link
Member

retest this please

@wangjia007bond
Copy link

The check of golanglint-ci fail, please fix them.

you can run golangci-lint run -c ./golangci.yml ./... in locally.

pulsar/test_helper.go:201:1: context.Context should be the first parameter of a function (golint)
func send(t *testing.T, producer Producer, ctx context.Context, times int, message *ProducerMessage) {
^
pulsar/test_helper.go:216:1: context.Context should be the first parameter of a function (golint)
func receive(t *testing.T, consumer Consumer, ctx context.Context, times int, inspect func(*testing.T, Message, int)) {
^
pulsar/test_helper.go:25: File is not `goimports`-ed (goimports)
	"github.com/stretchr/testify/assert"
pulsar/consumer_test.go:56: File is not `goimports`-ed (goimports)
	msgProperties := map[string]string{ "key-1": "pulsar-1" }

Done.

Copy link
Member

@wolfstudy wolfstudy left a comment

Choose a reason for hiding this comment

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

Nice work, thanks @stevenwangnarvar

@wolfstudy wolfstudy merged commit f41441f into apache:master Feb 3, 2020
@wolfstudy wolfstudy added this to the 0.1.0 milestone Mar 23, 2020
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.

4 participants