jineshparakh commented on code in PR #19407:
URL: https://github.com/apache/pinot/pull/19407#discussion_r3914754942
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ServerChannels.java:
##########
@@ -236,11 +237,45 @@ void sendRequest(String rawTableName, AsyncQueryResponse
asyncQueryResponse,
void connectWithoutLocking()
Review Comment:
Good catch, and you're right that it made the backward-compatibility claim
inaccurate. Fixed: `recordConnectTime()` sets both the gauge and the new timer,
and it is now called from `connectWithoutLocking()` as well — so the lazy path,
which is the only connect path when `enabled=false`, records exactly as it did
before.
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ServerChannels.java:
##########
@@ -236,11 +237,45 @@ void sendRequest(String rawTableName, AsyncQueryResponse
asyncQueryResponse,
void connectWithoutLocking()
throws InterruptedException {
+ // Lazy query path: open the TCP connection only, exactly as before this
feature existed. Any TLS
+ // handshake is left to proceed asynchronously so the channel lock is
released as soon as the socket
+ // is up, keeping the first query's critical section short.
+ if (_channel == null || !_channel.isActive()) {
+ _channel = _bootstrap.connect().sync().channel();
+ }
+ }
+
+ /// Like [#connectWithoutLocking()] but additionally waits out the TLS
handshake and records the
+ /// establish latency. Used only by startup pre-connect ([#connect()]), so
paying the handshake -- and
+ /// the longer critical section it implies -- never touches the lazy query
path. Runs under the channel
+ /// lock, like its sibling.
+ void connectAndAwaitHandshakeWithoutLocking()
+ throws InterruptedException {
if (_channel == null || !_channel.isActive()) {
long startTime = System.currentTimeMillis();
_channel = _bootstrap.connect().sync().channel();
-
_brokerMetrics.setValueOfGlobalGauge(BrokerGauge.NETTY_CONNECTION_CONNECT_TIME_MS,
- System.currentTimeMillis() - startTime);
+ awaitTlsHandshake(_channel);
+ long connectTimeMs = System.currentTimeMillis() - startTime;
+
_brokerMetrics.setValueOfGlobalGauge(BrokerGauge.NETTY_CONNECTION_CONNECT_TIME_MS,
connectTimeMs);
+
_brokerMetrics.addTimedValue(BrokerTimer.NETTY_CONNECTION_CONNECT_TIME,
connectTimeMs,
+ TimeUnit.MILLISECONDS);
+ }
+ }
+
+ /// Blocks until the TLS handshake on a freshly-connected channel
completes.
+ ///
+ /// `bootstrap.connect().sync()` returns once the TCP connection is up;
the client-mode [SslHandler]
+ /// then drives the handshake asynchronously on the event loop. Awaiting
its future here pays the
+ /// handshake -- two round trips plus certificate validation -- on the
connecting thread rather than
+ /// on the first query that writes to the channel. On a plaintext channel
there is no [SslHandler] in
+ /// the pipeline and this is a no-op. The calling thread is never an
event-loop thread, so this
+ /// cannot deadlock. A failed handshake propagates as an unchecked
exception, which the caller
+ /// treats exactly like a failed connect.
+ private void awaitTlsHandshake(Channel channel)
+ throws InterruptedException {
+ SslHandler sslHandler = channel.pipeline().get(SslHandler.class);
+ if (sslHandler != null) {
+ sslHandler.handshakeFuture().sync();
Review Comment:
Fixed. `_channel` is assigned only after `awaitTlsHandshake` returns; on
failure the channel is closed and the exception propagates, so a doomed channel
is never published.
Worth recording why the race is real rather than theoretical: in
`SslHandler#setHandshakeFailure`, `Promise#tryFailure` runs *before*
`SslUtils#handleHandshakeFailure` -> `ctx.close()`, and that close is itself
asynchronous. So a channel assigned up front genuinely can still report
`isActive()` at the moment the waiter throws.
On the 10s bound — rather than document the dependency I removed it. The
handshake is now awaited with `await(remainingBudget)` instead of `sync()`, so
it is bounded by the caller's deadline rather than by `SslHandler`'s default.
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ServerChannels.java:
##########
@@ -271,7 +306,7 @@ void connect()
throws InterruptedException, TimeoutException {
if (_channelLock.tryLock(TRY_CONNECT_CHANNEL_LOCK_TIMEOUT_MS,
TimeUnit.MILLISECONDS)) {
try {
- connectWithoutLocking();
+ connectAndAwaitHandshakeWithoutLocking();
Review Comment:
You're right, and the javadoc was simply false. Split as you suggested:
`connect()` is back on the TCP-only `connectWithoutLocking()`, and startup
pre-connect has its own `ServerChannels#preConnect` entry point that awaits the
handshake. No handshake is held under `_channelLock` on any path that runs
under live traffic.
One refinement on severity, for the record: unhealthy servers are removed
from routing (`BaseBrokerStarter` wires `registerUnhealthyServerNotifier` ->
`excludeServerFromRouting`), so the concurrent-query pile-up is narrower than
described. The residual that did matter is that the single
`failure-detector-retry` thread processes servers sequentially, so one hung TLS
peer would have delayed every *other* server's recovery by up to the handshake
timeout. Fixed either way.
--
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]
