-
Notifications
You must be signed in to change notification settings - Fork 335
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
[refactor]: Refactor the toTrackingMessageID() #972
Conversation
b8f29be
to
5e4f474
Compare
Hi @BewareMyPower @RobertIndie @nodece @shibd, could you give a look when you are free? |
/pulsarbot run-failure-checks |
Still doesn't work... |
The flaky test is related to #971 |
@BewareMyPower yes, rerun CI failed again. Looks like CI is stucked in |
e5fbffc
to
29b70f1
Compare
I will review it tomorrow |
if c.options.EnableDefaultNackBackoffPolicy || c.options.NackBackoffPolicy != nil { | ||
mid := c.messageID(msg.ID()) | ||
if mid == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we check the type of msgID, we can't remove this check, because there is other logic that returns nil
pulsar-client-go/pulsar/consumer_impl.go
Lines 755 to 759 in 48b7d01
if partition < 0 || partition >= len(c.consumers) { | |
c.log.Warnf("invalid partition index %d expected a partition between [0-%d]", | |
partition, len(c.consumers)) | |
return nil | |
} |
We can add checkMessageIdType
to c. messageID
method, and keep this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion! I will fix it.
Motivation
toTrackingMessageID()
is a function that is widely used inconsumer
implementation. It can convert an interface typeMessageID
into an struct typetrackingMessageID
. In addition, the second return value also plays a role in checking theMessageID
type. In other words, it indicates thatMessageID
cannot be a user-defined type. From the perspective of code readability,toTrackingMessageID()
should not do both.Note: After #968 ,
toTrackingMessageID()
returns only a pointer now. The role of originalok
is replaced by nil pointer now. However, the main content discussed in this PR has not changed.For example.
pulsar-client-go/pulsar/consumer_regex.go
Lines 176 to 181 in e2ea255
This example is the correct usage. The
ok
returned bytoTrackingMessageID()
is used to reject user-definedMessageID
.pulsar-client-go/pulsar/consumer_partition.go
Lines 470 to 473 in e2ea255
This example is a bit vague. The actual effect here is the same as the previous example. But it return an error
failed to convert trackingMessageID
which is confusing.pulsar-client-go/pulsar/consumer_partition.go
Lines 1816 to 1820 in e2ea255
In this case. We just want to convert
MessageID
intotrackingMessageID
. We do not care what it really is because it's not possible an invalidMessageID
implementation.So, original
toTrackingMessageID()
needs to require a careful look to use it correctly. I think it would be better to split it into two different method.toTrackingMessageID()
just do the struct conversion, which it's more clearly. And when the new messageID type is added, we can just modify thecheckMessageIDType
.Modifications
toTrackingMessageID()
checkMessageIDType()
to check whetherMessageID
is user-defined.Verifying this change
Documentation