github-actions[bot] commented on code in PR #69421:
URL: https://github.com/apache/doris/pull/69421#discussion_r4344451563
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java:
##########
@@ -779,6 +779,30 @@ public void setCommand(MysqlCommand command) {
this.command = command;
}
+ /** Reserve an idle MySQL connection before dispatching a newly received
packet. */
+ public synchronized boolean startMysqlCommand() {
+ if (isKilled) {
+ return false;
+ }
+ // COM_QUERY also covers the short interval before dispatch decodes
the command.
+ // Otherwise the shutdown thread could mistake a received packet for
an idle socket.
+ command = MysqlCommand.COM_QUERY;
+ return true;
+ }
+
+ /** Close an idle socket without cancelling an executor that has just
finished. */
+ public boolean closeIdleMysqlConnection() {
+ synchronized (this) {
+ if (getConnectType() != ConnectType.MYSQL || command !=
MysqlCommand.COM_SLEEP || isKilled) {
Review Comment:
[P2] Keep the busy reservation through reset response finalization
An admitted COM_RESET_CONNECTION can still be closed as idle here. Its
dispatch calls ConnectContext.resetConnection(), which sets command = COM_SLEEP
before handleResetConnection() sets OK and before processOnce() calls
finalizeCommand() to send the response. If the shutdown thread scans the
connection in that interval, this predicate marks it killed and closes the
socket even though its request is still counted as active. The client loses the
admitted reset's reply. Preserve a separate processing reservation through
response finalization, or keep resetConnection() from publishing COM_SLEEP
before that point.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ConnectScheduler.java:
##########
@@ -124,6 +124,22 @@ public Map<Integer, ConnectContext> getConnectionMap() {
return connectPoolMgr.getConnectionMap();
}
+ public void closeIdleMysqlConnections() {
+ for (ConnectContext context : getConnectionMap().values()) {
+ try {
+ context.closeIdleMysqlConnection();
+ } catch (Exception e) {
+ LOG.warn("failed to close idle MySQL connection {}",
context.getConnectionId(), e);
+ }
+ }
+ }
+
+ public boolean hasPendingFlightResults() {
+ return getConnectionMap().values().stream()
+ .anyMatch(context -> context.getConnectType() ==
ConnectContext.ConnectType.ARROW_FLIGHT_SQL
+ && context.getFlightSqlChannel().resultNum() > 0);
Review Comment:
[P2] Exclude expired Flight entries from the pending-result check
resultNum() returns Guava Cache.size(), which [can include expired
entries](https://guava.dev/releases/33.2.1-jre/api/docs/com/google/common/cache/CacheBuilder.html).
For example, leave a Flight session open after abandoning a FE-local result,
then shut down more than ten minutes later. The result has expired, but the
session can remain for the default eight-hour wait_timeout, and neither the
session timeout checker nor repeated size() calls cleans up this cache. This
predicate can therefore remain true until the full grace_shutdown_wait_seconds
elapses (300 seconds by default) even though no result can be retrieved and no
request or coordinator remains. Remove expired entries or inspect only live
results before using cache occupancy to hold shutdown open.
##########
fe/fe-core/src/main/java/org/apache/doris/DorisFE.java:
##########
@@ -726,19 +727,46 @@ public static boolean isServerReady() {
}
private static void gracefulShutdown() {
- // wait for all queries to finish
+ // Health is already 503, but keep serving during LB / DNS convergence.
try {
- long now = System.currentTimeMillis();
- List<Coordinator> allCoordinators =
QeProcessorImpl.INSTANCE.getAllCoordinators();
- while (!allCoordinators.isEmpty() && System.currentTimeMillis() -
now < 300 * 1000L) {
- Thread.sleep(1000);
- allCoordinators =
QeProcessorImpl.INSTANCE.getAllCoordinators();
- LOG.info("waiting {} queries to finish before shutdown",
allCoordinators.size());
+ LOG.info("graceful shutdown: serving during drain for {} seconds",
Config.graceful_shutdown_drain_seconds);
+ TimeUnit.SECONDS.sleep(Math.max(0,
Config.graceful_shutdown_drain_seconds));
Review Comment:
[P2] Skip the drain delay for processes that never served requests
The shutdown hook is installed before checkCommandLineOptions(), so
bin/start_fe.sh --version reaches System.exit(0) and this sleep while all
service listeners are still uninitialized. The same happens for diagnostic-tool
exits and startup failures after hook registration. These paths now wait 15
seconds by default, or the entire configured drain interval, despite never
having served traffic that needs load-balancer convergence. Apply this delay
only when the process had actually entered service, retaining that state before
the hook clears serverReady.
--
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]
