You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "Gleiphir2769 (via GitHub)" <gi...@apache.org> on 2023/02/28 15:36:57 UTC
[GitHub] [pulsar-client-go] Gleiphir2769 opened a new pull request, #972: [Refactor]: Refactor the toTrackingMessageID()
Gleiphir2769 opened a new pull request, #972:
URL: https://github.com/apache/pulsar-client-go/pull/972
### Motivation
`toTrackingMessageID()` is a function that is widely used in `consumer` implementation. It is to convert an interface type `MessageID` into an struct type `trackingMessageID`. But the second return value of this function is redundant and confusing.
For example.
https://github.com/apache/pulsar-client-go/blob/e2ea255052e8a527091791ef368851d885ee2d45/pulsar/consumer_partition.go#L470-L473
https://github.com/apache/pulsar-client-go/blob/e2ea255052e8a527091791ef368851d885ee2d45/pulsar/consumer_partition.go#L529-L532
https://github.com/apache/pulsar-client-go/blob/e2ea255052e8a527091791ef368851d885ee2d45/pulsar/consumer_partition.go#L588-L591
The second return value indicates whether the `MessageID` is one of the following types `trackingMessageID`, `messageID` or `chunkingMessageID`. But `toTrackingMessageID` is not a public function, the limitation of type is useless. Currently, the second return value will always be true.
https://github.com/apache/pulsar-client-go/blob/e2ea255052e8a527091791ef368851d885ee2d45/pulsar/impl_message.go#L235-L252
Therefore, I reimplemented `toTrackingMessageID` and redefines the meaning of the second return value. It means whether the returned `trackingMessageID` is converted from trackingMessageID. So it can indicate whether we need to pay attention to batch processing because `trackingMessageID` is actually a `batchMessageID` implementation.
### Modifications
- Refactor the `toTrackingMessageID()`
- Remove the redundant return value judgment in `consumer`, `partitionConsumer`...
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### 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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] Gleiphir2769 commented on pull request #972: [refactor]: Refactor the toTrackingMessageID()
Posted by "Gleiphir2769 (via GitHub)" <gi...@apache.org>.
Gleiphir2769 commented on PR #972:
URL: https://github.com/apache/pulsar-client-go/pull/972#issuecomment-1449206045
Hi @BewareMyPower @RobertIndie @nodece @shibd, could you give a look when you are free?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] Gleiphir2769 commented on pull request #972: [refactor]: Refactor the toTrackingMessageID()
Posted by "Gleiphir2769 (via GitHub)" <gi...@apache.org>.
Gleiphir2769 commented on PR #972:
URL: https://github.com/apache/pulsar-client-go/pull/972#issuecomment-1449245352
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] Gleiphir2769 commented on a diff in pull request #972: [refactor]: Refactor the toTrackingMessageID()
Posted by "Gleiphir2769 (via GitHub)" <gi...@apache.org>.
Gleiphir2769 commented on code in PR #972:
URL: https://github.com/apache/pulsar-client-go/pull/972#discussion_r1127257123
##########
pulsar/consumer_impl.go:
##########
@@ -580,12 +583,12 @@ func (c *consumer) ReconsumeLaterWithCustomProperties(msg Message, customPropert
}
func (c *consumer) Nack(msg Message) {
+ if !checkMessageIDType(msg.ID()) {
+ c.log.Warnf("invalid message id type %T", msg.ID())
+ return
+ }
if c.options.EnableDefaultNackBackoffPolicy || c.options.NackBackoffPolicy != nil {
mid := c.messageID(msg.ID())
- if mid == nil {
Review Comment:
Great suggestion! I will fix it.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] RobertIndie merged pull request #972: [refactor]: Refactor the toTrackingMessageID()
Posted by "RobertIndie (via GitHub)" <gi...@apache.org>.
RobertIndie merged PR #972:
URL: https://github.com/apache/pulsar-client-go/pull/972
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] Gleiphir2769 commented on pull request #972: [refactor]: Refactor the toTrackingMessageID()
Posted by "Gleiphir2769 (via GitHub)" <gi...@apache.org>.
Gleiphir2769 commented on PR #972:
URL: https://github.com/apache/pulsar-client-go/pull/972#issuecomment-1449624990
> The flaky test is related to #971
@BewareMyPower yes, rerun CI failed again. Looks like CI is stucked in `TestConsumerSeekByTimeOnPartitionedTopic` now.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] shibd commented on a diff in pull request #972: [refactor]: Refactor the toTrackingMessageID()
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on code in PR #972:
URL: https://github.com/apache/pulsar-client-go/pull/972#discussion_r1126174434
##########
pulsar/consumer_impl.go:
##########
@@ -580,12 +583,12 @@ func (c *consumer) ReconsumeLaterWithCustomProperties(msg Message, customPropert
}
func (c *consumer) Nack(msg Message) {
+ if !checkMessageIDType(msg.ID()) {
+ c.log.Warnf("invalid message id type %T", msg.ID())
+ return
+ }
if c.options.EnableDefaultNackBackoffPolicy || c.options.NackBackoffPolicy != nil {
mid := c.messageID(msg.ID())
- if mid == nil {
Review Comment:
Even if we check the type of msgID, we can't remove this check, because there is other logic that returns `nil`
https://github.com/apache/pulsar-client-go/blob/48b7d0195327a81f638a3c51df2010d7eb244bbe/pulsar/consumer_impl.go#L755-L759
We can add `checkMessageIdType` to `c. messageID` method, and keep this check.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] BewareMyPower commented on pull request #972: [refactor]: Refactor the toTrackingMessageID()
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #972:
URL: https://github.com/apache/pulsar-client-go/pull/972#issuecomment-1449373520
The flaky test is related to https://github.com/apache/pulsar-client-go/issues/971
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] BewareMyPower commented on pull request #972: [refactor]: Refactor the toTrackingMessageID()
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #972:
URL: https://github.com/apache/pulsar-client-go/pull/972#issuecomment-1453762345
I will review it tomorrow
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] Gleiphir2769 commented on pull request #972: [refactor]: Refactor the toTrackingMessageID()
Posted by "Gleiphir2769 (via GitHub)" <gi...@apache.org>.
Gleiphir2769 commented on PR #972:
URL: https://github.com/apache/pulsar-client-go/pull/972#issuecomment-1449245795
> /pulsarbot run-failure-checks
Still doesn't work...
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org