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

add LedgerId,EntryId,BatchIdx,PartitionIdx func for MessageId interface #529

Merged
merged 3 commits into from
Jun 1, 2021

Conversation

yorkhellen
Copy link
Contributor

In some cases, consumers need to obtain the ledgerId, entryId, batchId, partitionidx of the messageId.

This expose LedgerId EntryId BatchId PartitionIdx func for MessageId interface.

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.

Thanks @yorkhellen work for this, the change LGTM +1, just a little comments, please check?

assert.Equal(t, int64(2), id2.EntryId())
assert.Equal(t, int32(3), id2.BatchIdx())
assert.Equal(t, int32(4), id2.PartitionIdx())

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not cover the current function in the original test case. Can you add a new test case to cover this feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.

@wolfstudy wolfstudy added this to the 0.6.0 milestone Jun 1, 2021
"github.com/stretchr/testify/assert"
"testing"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here go's ci check should fail, please make sure that Action CI is passable

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.

LGTM +1

@wolfstudy wolfstudy merged commit a03349a into apache:master Jun 1, 2021
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.

None yet

2 participants