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 304][Reader] fixed panic in CreateReader API using custom MessageID for ReaderOptions #305

Merged
merged 2 commits into from Jul 6, 2020

Conversation

nitishv
Copy link
Contributor

@nitishv nitishv commented Jul 5, 2020

Fixes #304

Motivation

User of the client's client's CreateReader API can use a custom type satisfying the MessageID interface, when using it as a value for StartMessageID in ReaderOptions argument for the mentioned API.

The current reader creation does an untested type assertion here, when preparing the consumerOptions needed for creating a partitionConsumer.
https://github.com/apache/pulsar-client-go/blob/master/pulsar/reader_impl.go#L64

This assertion of MessageID as *messageID will fail unless an instance of MessageID is created from one of these exported APIs because messageID is unexported
https://github.com/apache/pulsar-client-go/blob/master/pulsar/message.go#L114-#L126
Note: newMessageID returns *messageID which satisfies MessageID interface as well.

Modifications

Test the type assertion of MessageID as *messageID, if it fails, re-create a new MessageID using this

func deserializeMessageID(data []byte) (MessageID, error) {

This will ensure that the custom type can be re-created as a *messageID which can be used by partitionConsumerOpts

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

Added unit test TestReaderOnSpecificMessageWithCustomMessageID in github.com/apache/pulsar-client-go/pulsar package.
Test uses a custom StartMessageID in ReaderOptions while creating a new reader to read from a specific message ID, rather than the pre-canned earliest/latest options.

CI script test result snippet below.

-- Ready to start tests
+ go test -race -coverprofile=/tmp/coverage -timeout=20m ./...
?       github.com/apache/pulsar-client-go/examples/consumer    [no test files]
?       github.com/apache/pulsar-client-go/examples/consumer-listener   [no test files]
?       github.com/apache/pulsar-client-go/examples/producer    [no test files]
?       github.com/apache/pulsar-client-go/examples/reader      [no test files]
ok      github.com/apache/pulsar-client-go/integration-tests    1.732s  coverage: 0.0% of statements
?       github.com/apache/pulsar-client-go/perf [no test files]

ok      github.com/apache/pulsar-client-go/pulsar       141.199s        coverage: 81.2% of statements
ok      github.com/apache/pulsar-client-go/pulsar/internal      1.658s  coverage: 30.7% of statements
ok      github.com/apache/pulsar-client-go/pulsar/internal/auth 1.028s  coverage: 31.3% of statements
ok      github.com/apache/pulsar-client-go/pulsar/internal/compression  1.041s  coverage: 55.7% of statements
?       github.com/apache/pulsar-client-go/pulsar/internal/pulsar_proto [no test files]
+ go tool cover -html=/tmp/coverage -o coverage.html

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

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

Documentation

  • Does this pull request introduce a new feature? : No

@nitishv
Copy link
Contributor Author

nitishv commented Jul 5, 2020

@wolfstudy @sijie @Ghatage please have a look

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.

Cool, Thanks @nitishv work for this, LGTM +1

@Ghatage
Copy link

Ghatage commented Jul 6, 2020

Great stuff @nitishv! Excited to have you involved in the Pulsar community!

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

Successfully merging this pull request may close these issues.

Go client Reader panics when passing custom StartMessageID
3 participants