[improve][broker] PIP-192 Added Deleted and Init states in ServiceUnitState

[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

To break this invalid transition, we need semi-terminal states(Deleted, and Free) and Init state to better represent tombstoned bundles.

Owned -> Splitting -> Deleted -> Transfer_Assigned // invalid
Owned -> Released -> Free -> Transfer_Assigned // invalid

Modifications

This PR

• added Deleted and Init States in ServiceUnitState.
• added the Owned- > Released transition to make the Unload command a two-phase protocol.
• scheduled monitorOwnerships() on the leader broker's ServiceUnitStateChannel, which overrides states to Owned or Init for service units(bundles)
  • from inactive brokers (to Owned)
  • in in-flight states too long (orphan bundles) (to Owned)
  • in semi-terminal states( Deleted and Free) long enough to tombstone (to Init).

Future Work / Discussion

Also, we may need to think about pre-creating all bundles in Owned state whenever a new namespace is created.

If all bundles are pre-created and do exist in the service units state channel, tombstone can only happen on parent bundles after split(Deleted state).

Also, with the bundle pre-creations and the transfer command, there will be no need for the assign command,(which could cause thundering herds impact when many brokers concurrently issue bundle ownership). This will simplify the state transitions by removing the 'Free' state and 'Free -> Assigned' transition.

However, this pre-creation requires some additional memory space from keeping bundles in Owned state(some might not be used at all).

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#32

[Technoboy-]

Why do we add waitForReconnection?

[heesung-sn]

When seek() resets the cursor, this reader will be temporarily disconnected.

Then, when calling acknowledgeCumulativeAsync() at the end of the compaction(below code), the reader might throw an exception because state == Connecting. This issue could likely happen if there is only one message to compact.

                .thenCompose(v -> {
                    log.info("Acking ledger id {}", phaseOneResult.firstId);
                    return ((CompactionReaderImpl<T>) reader)
                            .acknowledgeCumulativeAsync(
                                    phaseOneResult.lastId, Map.of(COMPACTED_TOPIC_LEDGER_PROPERTY,
                                            ledger.getId()));
                })

[gaoran10]

Great work! Left some comments.

[Demogorgon314]

LGTM.