[improve][broker] PIP-192 Use msg property to mark compacted msgs from strategic compaction

[heesung-sn]

Master Issue: #16691

Motivation

There is a possible edge case where bundle ownership could be in an invalid state in the new load balancer.

Let's say strategic compaction happened in the middle of the bundle transfer.
Owned->Transfer_Assigned // compaction happened.

After the compaction, when a tableview joins and builds its cache from the compacted topic, it will first see a transition, null -> Transfer_Assigned. However, because this is invalid, the tableview will skip this msg, causing an inconsistent view.

To avoid this issue, this transition(null -> Transfer_Assigned) has been changed to a valid transition in the state diagram, but I realized that this could bring another wrong state when transfer and split occur concurrently.

Racing condition
leader : Owned -> Transfer_Assigned -> Released -> Owned
Broker-2 : Owned -> Splitting -> null(parent-bundle)

Wrong state: a parent bundle is owned by another broker after the split.
Owned -> Splitting -> null -> Transfer_Assigned -> Released -> Owned

Ideally, we need to mark if messages are compacted or not. Then, TableView can know (null -> compacted_messages) are valid transitions.

null -> Transfer_Assigned(compacted) // valid transition.
null -> Transfer_Assigned(non-compacted) // invalid transition.

Modifications

This PR added a system property to compacted msgs from strategic compaction.

Verifying this change

• Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

• *Updaated unit 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
• Anything that affects deployment

Documentation

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

We will have separate PRs to update the Doc later.

Matching PR in forked repository

PR in forked repository: heesung-sn#31

[codelipenghui]

@heesung-sn I am concerned about "However, because this is invalid, the tableview will skip this msg, causing an inconsistent view."

If we skip the message when consuming the data, the consumer will only receive it again if the consumer disconnects. The read position of the cursor will move to the end of the topic, after the topic compaction task done, the compacted data will not dispatch to the consumer again. Is it will be a problem in this case?

[heesung-sn]

Good point.

we should change the condition and accept all compacted messages are valid transitions (even if the prev msg is not null).

[eolivelli]

why aren't we using StringUtils.equals for every comparison ?

[heesung-sn]

broker is never null, whereas sourceBroker can be null.

[heesung-sn]

@heesung-sn I am concerned about "However, because this is invalid, the tableview will skip this msg, causing an inconsistent view."
If we skip the message when consuming the data, the consumer will only receive it again if the consumer disconnects. The read position of the cursor will move to the end of the topic, after the topic compaction task done, the compacted data will not dispatch to the consumer again. Is it will be a problem in this case?

Good point.

we should change the condition and accept all compacted messages are valid transitions (even if the prev msg is not null).

I misunderstood what you said. Please ignore my above comment.

I will close this PR and propose another solution without msg properties because, as you commented, topic cursors can reset to compaction horizon, which won't have message properties.

[heesung-sn]

Raised a PR for an alternative solution.

#19546