Range aware port per broker exposition

[robobario]

Type of change

• Enhancement / new feature

Description

Add RangeAwarePortPerNodeClusterNetworkAddressConfigProvider

Mitigates #902

This new implementation introduces the concept of Node Id Ranges and enables you to configure multiple such ranges for your virtual cluster. Enabling a deterministic mapping of node ids to proxy ports even when there are large gaps between the ranges.

Ranges are specified in interval notation with square brackets for inclusive and round brackets for inclusive.

Each node id in the union of all the ranges (the ranges must be distinct and not intersect) is deterministically mapped to a port. To do this we effectively sort all the nodeIds into an ascending list and use the position in the list as an offset we then add to the start port.

For example if I configure a virtual cluster with:

nodeStartPort: 9000
nodeIdRanges:
  - name: brokers
    range: [4,6)
  - name: controllers
    range: [1,2]

Then the ports will be mapped as follows:

• node 1 -> port 9000
• node 2 -> port 9001
• node 4 -> port 9002
• node 5 -> port 9003

Why: Currently there are numerous drawbacks to using the PortPerBroker exposition scheme. If the lowest broker id is N and the highest is M then we must expose M-N deterministically map each node id to a port. So if you have two nodes with ids 100000 and 200000 you need 100000 ports mapped so that we can say startPort+0 is node 100000 and startPort+100000 is node id 200000.

Also with the advent of KRaft we now may have to contend with two distinct and evolving sets of node ids, a controller pool and a broker pool. If there is a significant gap between them we have the same problem as above.

This enables us to represent clusters with highly variable node ids using a more compact set of ports.

Note it still does not solve other issues, like some users regulurly introduce new nodes and remove old ones in their target cluster as part of upgrades. Leading to eternal growth of node ids. To handle this in a better way we think the Proxy would need to be able to discover the target topology.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

• [ ] Write tests
• [ ] Make sure all tests pass
• [ ] Review performance test results. Ensure that any degradations to performance numbers are understood and justified.
• [ ] Make sure all Sonarcloud warnings are addressed or are justifiably ignored.
• [ ] Update documentation
• [ ] Reference relevant issue(s) and close them after merging
• [ ] For user facing changes, update CHANGELOG.md (remember to include changes affecting the API of the test artefacts too).

[sonarqubecloud[bot]]

Quality Gate passed

Issues
55 New issues
0 Accepted issues

Measures
0 Security Hotspots
92.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[robobario]

One thing I'm not so sure about is the complexity of the interval notation. I can appreciate that it might be a commonly understood notation and gives users flexibility about whether they prefer an inclusive/exclusive end, but I wonder if something more verbose would be friendlier to machines. Like:

nodeIdRanges:
  - name: brokers
    startInclusive: 4
    endExclusive: 7

we don't need to mess with regexes/parsing, we get type safety for the ints from Jackson and it might be easier to build these ranges up programmatically, instead of the script having to template ints into a string.

WDYT?

[k-wall]

One thing I'm not so sure about is the complexity of the interval notation. I can appreciate that it might be a commonly understood notation and gives users flexibility about whether they prefer an inclusive/exclusive end, but I wonder if something more verbose would be friendlier to machines. Like:

nodeIdRanges:
  - name: brokers
    startInclusive: 4
    endExclusive: 7

we don't need to mess with regexes/parsing, we get type safety for the ints from Jackson and it might be easier to build these ranges up programmatically, instead of the script having to template ints into a string.

WDYT?

Yeah, maybe it is overkill.

It is disappointing that JDK doesn't have a Range. Guava does, but we don't have that as a runtime dependency. Jackson does have serializer for Guava Range serializer. Reimplementing what exists is dumb.

I do like the concise notation of the ranges. Guava is tried and trusted. Guava is already part of the Kafka Broker, and used by other related Kafka projects (some Strimzi components). Maybe adding it would be okay.

I'm 50/50.

[SamBarker]

it might be easier to build these ranges up programmatically, instead of the script having to template ints into a string.

I definitely think its easier to generate if we go for the more verbose option. It could just be me but I do find myself asking the question Does this bracket mean included or excluded? every time I read the concise syntax (I do it rarely enough that it leaves just enough doubt every time I do). So I also think the verbose option is easier for end users.

I do like the concise notation of the ranges. Guava is tried and trusted. Guava is already part of the Kafka Broker, and used by other related Kafka projects (some Strimzi components). Maybe adding it would be okay.

I don't have any objections to us including Guava at runtime but I'd argue that round tripping a single string/range config value is not enough to add it.

[robobario]

I don't have any objections to us including Guava at runtime but I'd argue that round tripping a single string/range config value is not enough to add it.

I'd suggest it's one that's likely to collide with users own dependencies so if we pull in guava that we might want to shade/package relocate it. edit: hmm, that likely goes for our other dependencies like jackson, so maybe it's no blocker. But yeah it feels like a bit much for a few range classes. Happy to go with their terminology though since range is the language in the JDK and guava.

[robobario]

Maybe we should start with the smaller PR offering startInclusive, endExclusive. It would still be open for adding a range: String property later as an improvement. So start with:

ranges:
  - name: brokers
    startInclusive: 1
    endExclusive: 4

and then later we could add range to the object if we wanted, mutually exclusive with the integers.

edit: opened opened #1220 which implements this.

[robobario]

Superceded by #1220