pulsar-client-cpp icon indicating copy to clipboard operation
pulsar-client-cpp copied to clipboard

[improve] change RoutingMode default from UseSinglePartition to RoundRobinDistribution

Open TakaHiR07 opened this issue 1 year ago • 6 comments

Motivation

It is more reasonable to set routingMode default RoundRobinDistribution, just as the java client.

Otherwise, user may ignore this config, and producer can not produce msg to all partitions

Modifications

change default routingMode config

Verifying this change

  • [ ] Make sure that the change passes the CI checks.

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

Documentation

  • [x] doc-required (Your PR needs to update docs and you will update later)

  • [ ] doc-not-needed (Please explain why)

  • [ ] doc (Your PR contains doc changes)

  • [ ] doc-complete (Docs have been already added)

TakaHiR07 avatar Jun 20 '23 09:06 TakaHiR07

I agree with @eolivelli. It would be good to fix the inconsistency with the Java client, but it would also bring breaking changes. We should discuss it on the mailing list.

BewareMyPower avatar Jun 20 '23 10:06 BewareMyPower

I agree that we should align the default with the Java client

can you please add a test case ?

This is breaking behaviour change, we should discuss it on the mailing list and we should also take care of not cherry-pick it on maintenance branches, such kind of changes is allowed only on major versions

Have modified the configuration test to verify default config change. It seems not necessary to add additional test because testRoundRobinRoutingPolicy() is included.

TakaHiR07 avatar Jun 21 '23 03:06 TakaHiR07

Hi, @TakaHiR07 is there any chance to resolve the conflicts?

BewareMyPower avatar Aug 16 '24 14:08 BewareMyPower

Hi, @TakaHiR07 is there any chance to resolve the conflicts?

@BewareMyPower done.

TakaHiR07 avatar Aug 20 '24 02:08 TakaHiR07

It seems that there are some existing tests that failed due to this change.

BewareMyPower avatar Aug 20 '24 13:08 BewareMyPower

I revert the pr change and this test "testHasMessageAvailableAfterSeekToEnd" still fail. It seems this pr is not the reason

TakaHiR07 avatar Aug 29 '24 09:08 TakaHiR07