wolfstudy commented on a change in pull request #745:
URL: https://github.com/apache/pulsar-client-go/pull/745#discussion_r825569875
##########
File path: Dockerfile
##########
@@ -17,8 +17,8 @@
# under the License.
#
-ARG GO_VERSION=golang:1.13
-FROM apachepulsar/pulsar:latest as pulsar
+ARG GO_VERSION=golang:1.15
+FROM apachepulsar/pulsar:2.8.1 as pulsar
Review comment:
Please do not make this modification, we may need to check why the
latest docker image cannot pass Action CI, and then create a corresponding
issue to fix this problem.
##########
File path: pulsar/internal/commands.go
##########
@@ -119,6 +121,20 @@ func (r *MessageReader) ReadMessageMetadata()
(*pb.MessageMetadata, error) {
return &meta, nil
}
+func (r *MessageReader) ReadBrokerMetadata() (*pb.BrokerEntryMetadata, error) {
+ magicNumber :=
binary.BigEndian.Uint16(r.buffer.Get(r.buffer.ReaderIndex(), 2))
+ if magicNumber != magicBrokerEntryMetadata {
+ return nil, nil
+ }
Review comment:
Why is the error object returned as nil here? If we don't need this
error field, maybe we can cancel it in function definition? Or we can use
`errors.new("xxx")` to customize the error message
##########
File path: pulsar/consumer_partition.go
##########
@@ -597,7 +602,18 @@ func (pc *partitionConsumer) MessageReceived(response
*pb.CommandMessage, header
pc.AckID(msgID)
continue
}
-
+ var messageIndex *uint64
+ var brokerPublishTime *time.Time
+ if brokerMetadata != nil {
Review comment:
It looks like we've checked above that the brokerMetadata object is nil
by doing the following:
```
brokerMetadata, err := reader.ReadBrokerMetadata()
if err != nil { // here
// todo optimize use more appropriate error codes
pc.discardCorruptedMessage(pbMsgID,
pb.CommandAck_BatchDeSerializeError)
return err
}
```
Do we still need to double check here? Will someone concurrently modify the
value of the brokerMetadata object?
##########
File path: pulsar/consumer_partition.go
##########
@@ -597,7 +602,18 @@ func (pc *partitionConsumer) MessageReceived(response
*pb.CommandMessage, header
pc.AckID(msgID)
continue
}
-
+ var messageIndex *uint64
+ var brokerPublishTime *time.Time
Review comment:
Maybe instead of using pointers we can use the following definition:
```
var messageIndex uint64
var brokerPublishTime time.Time
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
