rzo1 commented on PR #8892:
URL: https://github.com/apache/storm/pull/8892#issuecomment-4938416103
Thx for the PR. The overall design looks good to me: opt-in, default off and
well tested. A few things I would like to see changed before merging:
1. Please simplify `Constants.isControlStreamId`. The hand-rolled bitmask
fast path is a lot of subtle static-init machinery to avoid a single
`Set.contains` on a short string. Unless a benchmark showed this matters, a
plain `streamId != null && streamId.startsWith("__") &&
SYSTEM_CONTROL_STREAM_IDS.contains(streamId)` is easier to maintain. If you
keep the fast path, please add a short note on why it is needed.
2. Document the tick tuple drop semantics in the `Config` javadoc and in
`docs/Performance.md`. With the lane enabled, a full lane silently drops a
`__tick` tuple that previously would have blocked until delivered. User bolts
can rely on tick tuples for windowing or expiry logic, so please state
explicitly that user-facing tick tuples may occasionally be dropped under
saturation, not just internal signals.
3. Add a rate-limited WARN log when a control tuple is dropped in
`tryPublishControl`. A nonzero `control_dropped_messages` means the executor
thread is stalled, which operators should see in the logs and not only if they
happen to chart the new gauge.
4. Please add a short note (javadoc on the new constructor or on the field)
that `close()` intentionally does not drain the control lane, matching the
existing recvQueue behavior, so this is documented rather than implicit.
5. Nit: the new imports in `JCQueueTest` are slightly out of order
(`java.util.concurrent.atomic.AtomicInteger` before
`java.util.concurrent.ExecutorService`), checkstyle may flag this.
The remaining behavioral risks (control tuples overtaking co-enqueued data,
drop instead of block) are fine with me as long as they are documented as per
point 2.
--
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]
