jineshparakh opened a new pull request, #19567:
URL: https://github.com/apache/pinot/pull/19567
## Intent
`ExternalViewReader.getTableToBrokersMap()` builds the table-to-broker
routing map that
`DynamicBrokerSelector` refreshes from, and it re-reads the same data from
ZooKeeper many
times over.
The broker resource lives in a single znode, but it names brokers by Helix
instance ID. Turning an
instance ID into a connectable address means reading that broker's own
config znode under
`/CONFIGS/PARTICIPANT`. The method walks the broker resource table by table,
and resolves the
address inline for every table/broker pair it meets — so a broker serving a
thousand tables has its
config read a thousand times, and every read after the first returns the
same bytes.
A broker's address is a property of the broker. It does not depend on which
table is being examined,
so all but one of those reads is waste. For N tables each served by M
brokers the method performs
N x M reads where M would do, and each one is a separate sequential round
trip to the
ZooKeeper ensemble.
This matters because of when it happens. The map is rebuilt on every
broker-resource watch fire —
that is, precisely when routing has just changed and a client most needs to
converge quickly. On a
large cluster the redundant reads add seconds to that convergence, during
which the client is still
routing on the old map.
## What this changes
`getTableToBrokersMap()` now resolves each distinct broker's address once
per call and reuses it for
the remaining tables that broker serves. Reads drop from N x M to M.
The map used for this is created per call and passed in, rather than held as
a field. That is
deliberate on two counts. The method is called concurrently, so a shared
field would need
synchronising. And an address cached across calls would have to be
invalidated whenever a broker
changed host or port, which would mean watching a second znode; a per-call
map cannot go stale,
because the next refresh resolves everything again from scratch.
## Behaviour
The returned map is unchanged — same keys, same addresses, same ordering
semantics. The loop still
visits every table/broker pair and still collects into the same structures;
only the number of
ZooKeeper reads differs.
There is exactly one case where old and new observably differ. If a broker's
instance config changes
*while* a single traversal is in progress, the previous code would resolve
that broker to its old
address for tables visited before the change and its new address for tables
visited after, producing
a map in which one broker holds two different addresses. Reusing the first
resolution gives one
address per broker for the whole snapshot. This is a more coherent result,
and either way the next
refresh picks up the new address.
Exception handling is unaffected: a ZooKeeper failure still propagates out
of the resolution to the
same enclosing handler and aborts the traversal exactly as before, and no
partial entry is recorded
when it does.
## Scope
`getLiveBrokers()` on the same class walks the broker resource the same way
and carries the same
redundancy, and is fixed here too. It currently has no callers in the
repository, so this is not
where the win is — but the defect is identical and sits twenty lines from
the one that matters, and
leaving one of the two behind would only invite the same change again later
against a method whose
behaviour is then harder to reason about.
Its result is deliberately untouched. `getLiveBrokers()` returns a `List`
and has always emitted one
entry per table/broker pair rather than one per broker, so a broker serving
many tables appears many
times. Resolving the address once must not become an excuse to de-duplicate
that list: it is public
API, and a caller choosing at random would see its weighting change. Whether
the duplicates are
desirable is a separate question and not one this change answers.
## Test plan
Five tests added to `ExternalViewReaderTest`. The existing fixtures hold a
single table and a single
broker, where N x M and M are both 1, so none of them could observe this;
the new ones use two
tables served by the same two brokers — four pairs over two distinct brokers.
- `testGetTableToBrokersMapReadsEachInstanceConfigOncePerCall` — each
broker's config znode is read
exactly once, not once per table.
- `testGetLiveBrokersReadsEachInstanceConfigOncePerCall` — the same, for the
other method.
- `testGetTableToBrokersMapResultIsUnchangedByReusingResolvedAddresses` —
both tables still map to
both broker addresses.
- `testGetLiveBrokersStillReturnsOneEntryPerTableBrokerPair` — the returned
list keeps all four
entries, guarding the de-duplication trap described above.
- `testResolvedAddressesAreNotRetainedAcrossCalls` — a second call re-reads,
so nothing can be
served stale.
Results:
- Full `pinot-java-client` suite: **190 tests, 0 failures**, including
`DynamicBrokerSelectorTest`
and `BrokerCacheTest` — the consumers of this method, not just the
reader's own tests.
- All five new tests were confirmed load-bearing: restoring the inline
resolution fails the
read-count tests, and de-duplicating the list fails the pair-count test.
- `checkstyle` and `license` pass on `pinot-clients/pinot-java-client`.
### Measured against a live cluster
A quickstart was extended to 76 table resources served by 2 brokers — 152
table/broker pairs over 2
distinct brokers — and `getTableToBrokersMap()` was timed against its
ZooKeeper, before and after,
from the same harness. Both runs warm up first and report the median of 15
iterations.
| Build | median | min | mean |
|---|---|---|---|
| before | 28.3 ms | 27.3 ms | 28.1 ms |
| after | 1.3 ms | 1.0 ms | 1.3 ms |
The harness also prints a canonical digest of the returned map. It is
identical across both builds
(76 tables, same digest), so the result is unchanged and only the time
differs.
Note this is a ZooKeeper on loopback, where a read is well under a
millisecond. The saving is one
round trip per redundant read, so on a cluster whose client is a network hop
from the ZooKeeper
ensemble the absolute difference is larger, and it grows with the number of
tables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
