014 - Proposal to refactor frontend handler and centralize session context#88
Conversation
Refactor frontend handler architecture to improve separation of concerns, introduce a centralized session context, fix HAProxy message memory leak, and simplify the state machine by eliminating unnecessary states. Signed-off-by: Hrishabh Gupta <hgupta@confluent.io>
tombentley
left a comment
There was a problem hiding this comment.
This seems mostly reasonable. Thanks.
As a more general point, please don't feel the need to write a proposal for a refactoring which doesn't touch public APIs (which is the case here). A proposal can serve as a place to discuss a larger change without actually having to implement it in its entirety so you can open a PR for it.
I've also been thinking about a few other changes which would function as either clean up of accumulated tech debt (which we should do anyway) or enablers for routing-like functionality:
- Currently we initialize the backend pipeline in the frontend handler. That seems ripe for moving to the backend handler itself. I hope to open a PR for this next week.
- I think we should be able to factor out some kind of abstraction for tracking reasons and managing the
autoReadflag which we use for backpressure. I hope to open a PR for this next week. - I don't think the PCSM can really survive in its current form if we want to support routing. It seems to me there's a state machine associated with the frontend (tracking the building up of your
ClientSession), and one associated with a backend. A router could only forward (most) requests to a backend once we have TCP, TLS?, SASL?. At that point the Route is in theFORWARDINGstate, and requests can pass down the pipeline associated with that Route. Does this fit whatever thoughts you might have had about routing?
| ### HAProxy message memory leak | ||
|
|
||
| `HAProxyMessage` implements Netty's `ReferenceCounted` interface but is never released. The message is intercepted in `HAProxyMessageHandler`, passed to the state machine, stored in state records, but never has `release()` called. This causes a memory leak. |
There was a problem hiding this comment.
This seems like low-hanging fruit which doesn't depend on the rest. We should just fix it. Would you be willing to open a PR for this?
There was a problem hiding this comment.
I will take that up soon with the HAProxyMessage fix.
This introduces a proposal index (README.md) for the proposals directory to solve the problem of colliding proposal numbers. Currently, multiple open PRs have chosen proposal numbers that conflict with already-merged proposals. This creates confusion and makes it difficult to maintain stable identifiers for proposals - which is essential for productive discussion on the mailing list. The new process: 1. Authors create proposals with temporary filenames (nnn-*) 2. A separate PR to this README allocates the next sequential number 3. Authors rename their proposal once the number is allocated 4. The index is updated again when proposals are accepted This ensures: - Proposal numbers are allocated chronologically - No number collisions occur - Each proposal gets a stable identifier before mailing list discussion - Clear visibility of open vs. accepted proposals Current collisions resolved by this index: - PR kroxylicious#70, kroxylicious#82, kroxylicious#83, kroxylicious#88, kroxylicious#89 all have numbers conflicting with accepted proposals and have been allocated new sequential numbers (016-024) Assisted-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This introduces a proposal index (README.md) for the proposals directory to solve the problem of colliding proposal numbers. Currently, multiple open PRs have chosen proposal numbers that conflict with already-merged proposals. This creates confusion and makes it difficult to maintain stable identifiers for proposals - which is essential for productive discussion on the mailing list. The new process: 1. Authors create proposals with temporary filenames (nnn-*) 2. A separate PR to this README allocates the next sequential number 3. Authors rename their proposal once the number is allocated 4. The index is updated again when proposals are accepted This ensures: - Proposal numbers are allocated chronologically - No number collisions occur - Each proposal gets a stable identifier before mailing list discussion - Clear visibility of open vs. accepted proposals Current collisions resolved by this index: - PR kroxylicious#70, kroxylicious#82, kroxylicious#83, kroxylicious#88, kroxylicious#89 all have numbers conflicting with accepted proposals and have been allocated new sequential numbers (016-024) Assisted-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Keith Wall <kwall@apache.org>
This introduces a proposal index (README.md) for the proposals directory to solve the problem of colliding proposal numbers. Currently, multiple open PRs have chosen proposal numbers that conflict with already-merged proposals. This creates confusion and makes it difficult to maintain stable identifiers for proposals - which is essential for productive discussion on the mailing list. The new process: 1. Authors create proposals with temporary filenames (nnn-*) 2. A separate PR to this README allocates the next sequential number 3. Authors rename their proposal once the number is allocated 4. The index is updated again when proposals are accepted This ensures: - Proposal numbers are allocated chronologically - No number collisions occur - Each proposal gets a stable identifier before mailing list discussion - Clear visibility of open vs. accepted proposals Current collisions resolved by this index: - PR kroxylicious#70, kroxylicious#82, kroxylicious#83, kroxylicious#88, kroxylicious#89 all have numbers conflicting with accepted proposals and have been allocated new sequential numbers (016-024) Assisted-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Keith Wall <kwall@apache.org>
Refactor frontend handler architecture to improve separation of concerns, introduce a centralized session context, fix HAProxy message memory leak, and simplify the state machine by eliminating unnecessary states.