You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/04/12 13:22:43 UTC
[GitHub] [pulsar-client-go] cornelk opened a new pull request #214: Fix
Consumer Seek not returning a potential error
cornelk opened a new pull request #214: Fix Consumer Seek not returning a potential error
URL: https://github.com/apache/pulsar-client-go/pull/214
### Motivation
Review of Consumer Seek() did reveal a potential error that is not returned as such.
### Modifications
- refactored `messageID()` to not use a logger but as a helper function that returns an error. It can later be used by a currently missing `Reader` `Seek()` implementation.
- return a potential error from `Consumer.Seek()`
- log the error in `AckID()` and `NackID()` - changing the interface to return an error for those function should be a better approach
### Verifying this change
- [x] Make sure that the change passes the CI checks.
This change is already covered by existing tests
### 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): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
### Documentation
- Does this pull request introduce a new feature? (no)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request
#214: Fix Consumer Seek not returning a potential error
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #214: Fix Consumer Seek not returning a potential error
URL: https://github.com/apache/pulsar-client-go/pull/214#discussion_r408275588
##########
File path: pulsar/consumer_impl.go
##########
@@ -283,8 +284,9 @@ func (c *consumer) Nack(msg Message) {
}
func (c *consumer) NackID(msgID MessageID) {
- mid, ok := c.messageID(msgID)
- if !ok {
+ mid, err := newMessageIDFromInterface(msgID, len(c.consumers))
Review comment:
Why is that error needed? It looks like it's just being logged and not returned to the client?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request
#214: Fix Consumer Seek not returning a potential error
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #214: Fix Consumer Seek not returning a potential error
URL: https://github.com/apache/pulsar-client-go/pull/214#discussion_r408275605
##########
File path: pulsar/impl_message.go
##########
@@ -118,6 +120,24 @@ func newMessageID(ledgerID int64, entryID int64, batchIdx int, partitionIdx int)
}
}
+// creates a new messageID object from the given MessageID, consumers is the amount of
+// consumers, 1+ for Consumer, 1 for Reader.
+func newMessageIDFromInterface(msgID MessageID, consumers int) (*messageID, error) {
Review comment:
The method name is confusing to me something with new should return a new object. This is just casting it to something.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request
#214: Fix Consumer Seek not returning a potential error
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #214: Fix Consumer Seek not returning a potential error
URL: https://github.com/apache/pulsar-client-go/pull/214#discussion_r408275605
##########
File path: pulsar/impl_message.go
##########
@@ -118,6 +120,24 @@ func newMessageID(ledgerID int64, entryID int64, batchIdx int, partitionIdx int)
}
}
+// creates a new messageID object from the given MessageID, consumers is the amount of
+// consumers, 1+ for Consumer, 1 for Reader.
+func newMessageIDFromInterface(msgID MessageID, consumers int) (*messageID, error) {
Review comment:
The method name is confusing to me. Something with new should return a new struct. This is just casting it to something. Why is this method needed? If you are just looking to log the error why not just update the existing method?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request
#214: Fix Consumer Seek not returning a potential error
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #214: Fix Consumer Seek not returning a potential error
URL: https://github.com/apache/pulsar-client-go/pull/214#discussion_r408275605
##########
File path: pulsar/impl_message.go
##########
@@ -118,6 +120,24 @@ func newMessageID(ledgerID int64, entryID int64, batchIdx int, partitionIdx int)
}
}
+// creates a new messageID object from the given MessageID, consumers is the amount of
+// consumers, 1+ for Consumer, 1 for Reader.
+func newMessageIDFromInterface(msgID MessageID, consumers int) (*messageID, error) {
Review comment:
The method name is confusing to me. Something with new should return a new struct. This is just casting it to something. Why is this method needed? If you are just looking log the error why not just update the existing method?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cornelk commented on a change in pull request
#214: Fix Consumer Seek not returning a potential error
Posted by GitBox <gi...@apache.org>.
cornelk commented on a change in pull request #214: Fix Consumer Seek not returning a potential error
URL: https://github.com/apache/pulsar-client-go/pull/214#discussion_r407935900
##########
File path: pulsar/impl_message.go
##########
@@ -118,6 +120,24 @@ func newMessageID(ledgerID int64, entryID int64, batchIdx int, partitionIdx int)
}
}
+// creates a new messageID object from the given MessageID, consumers is the amount of
+// consumers, 1+ for Consumer, 1 for Reader.
+func newMessageIDFromInterface(msgID MessageID, consumers int) (*messageID, error) {
Review comment:
msgID is an interface, the returned value the internal struct
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cornelk commented on a change in pull request
#214: Fix Consumer Seek not returning a potential error
Posted by GitBox <gi...@apache.org>.
cornelk commented on a change in pull request #214: Fix Consumer Seek not returning a potential error
URL: https://github.com/apache/pulsar-client-go/pull/214#discussion_r407935419
##########
File path: pulsar/consumer_impl.go
##########
@@ -283,8 +284,9 @@ func (c *consumer) Nack(msg Message) {
}
func (c *consumer) NackID(msgID MessageID) {
- mid, ok := c.messageID(msgID)
- if !ok {
+ mid, err := newMessageIDFromInterface(msgID, len(c.consumers))
Review comment:
if the `msgID` interface paramter is not of the internal type `messageID` `newMessageIDFromInterface()` will fail
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cornelk commented on a change in pull request
#214: Fix Consumer Seek not returning a potential error
Posted by GitBox <gi...@apache.org>.
cornelk commented on a change in pull request #214: Fix Consumer Seek not returning a potential error
URL: https://github.com/apache/pulsar-client-go/pull/214#discussion_r410158809
##########
File path: pulsar/impl_message.go
##########
@@ -118,6 +120,24 @@ func newMessageID(ledgerID int64, entryID int64, batchIdx int, partitionIdx int)
}
}
+// creates a new messageID object from the given MessageID, consumers is the amount of
+// consumers, 1+ for Consumer, 1 for Reader.
+func newMessageIDFromInterface(msgID MessageID, consumers int) (*messageID, error) {
Review comment:
I renamed and moved the function so that it later can be used by `Reader` as well. This function existed before, it just moved. It converts the given interface implementation to the internal type. If this conversion fails, the callers like NackID can not succeed and should return an error to their caller. I propose this change instead of logging and not returning an error. This would be an API change tho.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request
#214: Fix Consumer Seek not returning a potential error
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #214: Fix Consumer Seek not returning a potential error
URL: https://github.com/apache/pulsar-client-go/pull/214#discussion_r407737101
##########
File path: pulsar/impl_message.go
##########
@@ -118,6 +120,24 @@ func newMessageID(ledgerID int64, entryID int64, batchIdx int, partitionIdx int)
}
}
+// creates a new messageID object from the given MessageID, consumers is the amount of
+// consumers, 1+ for Consumer, 1 for Reader.
+func newMessageIDFromInterface(msgID MessageID, consumers int) (*messageID, error) {
Review comment:
This is not returning a new message id it's the same struct right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request
#214: Fix Consumer Seek not returning a potential error
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #214: Fix Consumer Seek not returning a potential error
URL: https://github.com/apache/pulsar-client-go/pull/214#discussion_r407738309
##########
File path: pulsar/consumer_impl.go
##########
@@ -283,8 +284,9 @@ func (c *consumer) Nack(msg Message) {
}
func (c *consumer) NackID(msgID MessageID) {
- mid, ok := c.messageID(msgID)
- if !ok {
+ mid, err := newMessageIDFromInterface(msgID, len(c.consumers))
Review comment:
I'm confused what is the missing error?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services