[fix][broker] Partitioned shadow topic not work properly

[RobertIndie]

Motivation

Currently, when we create a shadow topic for a partitioned topic, the shadow topic won't get any messages from the source topic.

Root cause

Suppose we have a shadow topic test-shadow which shadows messages from the source topic test. And they should all be the partitioned topic. When initializing the ShadowManagedLedger for the test-shadow-partition-0, it will create a ShadowReplicator that gets messages from test-partition-0 but replicates messages to the wrong topic test-shadow. Therefore the shadow topic won't get any messages.

Solution

The solution is to create each ShadowReplicator replicate message from test-partition-X to test-shadow-partition-X. In this way, we could also ensure the message ordering.

However, the shadow topic must have more partitions than the source topic. When the source topic expands its number of partitions, we cannot automatically increase the partitions of the shadow topic because it may be in a different namespace with distinct permissions and configurations.

Therefore, I suggest that before expanding the source topic, we should first check if all associated shadow topics have been expanded. If not, we should throw an error to inform the user to expand all shadow topics first. Also, when setting up the shadow topic for the source topic, avoid adding "-partition-N" at the end. Otherwise, the ShadowReplicator will not function properly. And in that case, we should also throw the exception to inform the user.

After the discussion, I will split this solution into two parts:

• Part 1: fix the partitioned shadow topic support
• Part 2: Prevent expansion of a partitioned topic if its partition count exceeds that of any of its shadow topics. Prevent setting a partition topic ending with -partition-X as the shadow topic.

This PR will only handle the part 1.

Modifications

• Fix shadow replicator to support partitioned shadow topic

The following modifications will be split as a separate PR:

• Prevent expansion of a partitioned topic if its partition count exceeds that of any of its shadow topics.
• Prevent setting a partition topic ending with -partition-X as the shadow topic.

Verifying this change

This change added tests.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository:

[github-actions]

@RobertIndie Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

[liangyepianzhou]

Prevent expansion of a partitioned topic if its partition count exceeds that of any of its shadow topics.
Prevent setting a partition topic ending with -partition-X as the shadow topic.

Do we have a discussion about it?

[RobertIndie]

Do we have a discussion about it?

The discussion happens on this PR currently.
I pinged more people for border review.

[liangyepianzhou]

Do we have a discussion about it?

The discussion happens on this PR currently.
I pinged more people for border review.

I mean this should be discussed in the mail list. There are some behavior change.

new comments

[BewareMyPower]

@liangyepianzhou IMO, if it didn't add some validations to the existing updatePartitionedTopic API, it would not affect any existing behavior but only add the support for partitioned shadow topics. See the 1st part I mentioned here. /cc @RobertIndie

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.34%. Comparing base (bbc6224) to head (e5828c4).
Report is 421 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #22797      +/-   ##
============================================
- Coverage     73.57%   73.34%   -0.24%     
- Complexity    32624    33196     +572     
============================================
  Files          1877     1901      +24     
  Lines        139502   142526    +3024     
  Branches      15299    15573     +274     
============================================
+ Hits         102638   104529    +1891     
- Misses        28908    29980    +1072     
- Partials       7956     8017      +61     

Flag	Coverage Δ
inttests	27.46% <0.00%> (+2.87%)	⬆️
systests	24.74% <0.00%> (+0.41%)	⬆️
unittests	72.37% <100.00%> (-0.47%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...sar/broker/service/persistent/PersistentTopic.java	79.19% <100.00%> (+0.73%)	⬆️

... and 465 files with indirect coverage changes