fix: fix print message id without batch index

[Chenxulin97]

(If this PR fixes a github issue, please add Fixes #<xyz>.)

Fixes #

(or if this PR is one task of a github issue, please add Master Issue: #<xyz> to link to the master issue.)

Master Issue: #

Motivation

I found that different payload had the same message id

Modifications

append batch index when call string method of MessageID

Verifying this change

• Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

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): (yes / no)
• The public API: (yes / no)
• The schema: (yes / no / don't know)
• The default values of configurations: (yes / no)
• The wire protocol: (yes / no)

Documentation

• Does this pull request introduce a new feature? (yes / no)
• If yes, how is the feature documented? (not applicable / docs / GoDocs / not documented)
• If a feature is not applicable for documentation, explain why?
• If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

[shoothzj]

if batchIndex = -1, do we still need to print it?

[RobertIndie]

Let's keep it consistent with the Java client. Don't print the batch index when it's -1.

[Chenxulin97]

ok

[RobertIndie]

Please add the unit test case for it.

[RobertIndie]

Let's keep it consistent with the Java client. Don't print the batch index when it's -1.

[shoothzj]

@RobertIndie, do we really need a unit test case for the String() method? In my opinion, simply testing the String() method has less value.

[RobertIndie]

do we really need a unit test case for the String() method? In my opinion, simply testing the String() method has less value.

I think it's worth adding these tests. This is a public method exposed to users, and we need to ensure the correctness of the returned content. Moreover, other tests also rely on this String's output. We can see that this change has caused failures in other tests in the CI.

[shoothzj]

do we really need a unit test case for the String() method? In my opinion, simply testing the String() method has less value.

I think it's worth adding these tests. This is a public method exposed to users, and we need to ensure the correctness of the returned content. Moreover, other tests also rely on this String's output. We can see that this change has caused failures in other tests in the CI.

I understand the importance of writing test cases, and I won't insist on not writing them. However, I'd like to note that this part of the code logic is quite clear and straightforward.

[RobertIndie]

However, I'd like to note that this part of the code logic is quite clear and straightforward.

We write tests not because the code is clear or complex, but to ensure the functionality behaves correctly. Even for simple code, I believe it's best to write tests to cover it. This is exactly why the metric of test coverage is important.

[shoothzj]

@RobertIndie Fair enough, thanks for your explanation. I won't block this review.

[geniusjoe]

I think the unit test case ran fail is because TestGetMessagesByID function in pulsaradmin/pkg/admin/subscription_test.go use message id.String() as a map key to check if current messages are in a same batch.
When we change id.String() from fmt.Sprintf("%d:%d:%d", id.ledgerID, id.entryID, id.partitionIdx)
to fmt.Sprintf("%d:%d:%d:%d", id.ledgerID, id.entryID, id.partitionIdx, id.batchIdx), map key changed, so that the test case cannot distinguish messages in the same batch.

	for i := 0; i <= numberMessages; i++ {
		producer.SendAsync(ctx, &pulsar.ProducerMessage{
			Payload: []byte(fmt.Sprintf("hello-%d", i)),
		}, func(id pulsar.MessageID, message *pulsar.ProducerMessage, err error) {
			assert.Nil(t, err)
			messageIDMap[id.String()]++
			wg.Done()
		})
	}

If we need to fix test case, we can replace messageIDMap[id.String()] to messageIDMap[fmt.Sprintf("%d:%d:%d", id.ledgerID, id.entryID, id.partitionIdx)], but I think this pr might be an incompatibility change. Some users may use msg id.String() as an unique key or something else, so it should be better to add change log in documentation.

[Chenxulin97]

It seems more works than I expect, I have no time to work on this.