xiangfu0 commented on code in PR #20105:
URL: https://github.com/apache/pinot/pull/20105#discussion_r4427510019
##########
kb/code-review-principles.md:
##########
@@ -474,6 +474,40 @@ Not per-method arguments. Keep method signatures clean.
Data holders should not be named "Manager". Method names returning config
objects should include "Config" suffix.
- Trigger: Any PR introducing new classes or renaming existing ones
+**C3.25 — Size the ZooKeeper cost of a Helix listener that runs on every
instance**
+Helix prefetch is on by default. On each change, every listening process reads
all znodes under the watched path.
+With L listening processes and N znodes, one change costs L × N ZK reads on
the Helix event thread. A burst of
+changes, for example a scale-down, pays this cost for each change. If only the
query path needs the data, read it
+on the broker and send it with the request. If every instance needs the
listener, disable prefetch and read only
+the znode that changed.
+- Trigger: Any PR that adds a Helix change listener (`add*ChangeListener`,
`implements *ChangeListener`,
+ `ClusterChangeHandler`) that runs on every server or broker, or a full read
(`HelixHelper.getInstanceConfigs`,
+ `getChildValues`) inside a change callback
+
+```java
+// BAD: runs on every server. Each InstanceConfig change makes every server
read all InstanceConfigs.
+public class PeerPortCache implements InstanceConfigChangeListener {
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs,
NotificationContext context) {
+ _ports = buildPortMap(instanceConfigs); // Helix read all N
InstanceConfigs to build this list
+ }
+}
+```
+```java
+// GOOD: prefetch off. One ZK read per change. A full read only at start, or
when instances join or leave.
+@PreFetch(enabled = false)
Review Comment:
Please add `@BatchMode(enabled = false)` here, as every Pinot listener that
turns prefetch off already does (`ClusterChangeMediator`,
`ServerMailboxChannelRefreshHandler`, `SendStatsPredicate.Safe`,
`PinotHelixResourceManager`). Batch mode can be turned on JVM-wide with a
system property. When it is on, `CallbackEventExecutor` dedups queued events by
`Type`, so a burst of CALLBACKs keeps only one. That drops the
`getPathChanged()` of the other events, and it can drop an `isChildChange`
event too. The per-path `reload` then misses updates. A short note on why both
annotations are needed would help.
--
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]
