-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[fix](protocol) Prevent Connector/J cursor fetch from hanging on empty results #67520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
f83d84f
2c9fcd9
4da5164
12e48c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| // Licensed to the Apache Software Foundation (ASF) under one | ||
| // or more contributor license agreements. See the NOTICE file | ||
| // distributed with this work for additional information | ||
| // regarding copyright ownership. The ASF licenses this file | ||
| // to you under the Apache License, Version 2.0 (the | ||
| // "License"); you may not use this file except in compliance | ||
| // with the License. You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, | ||
| // software distributed under the License is distributed on an | ||
| // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| // KIND, either express or implied. See the License for the | ||
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| package org.apache.doris.mysql; | ||
|
|
||
| import com.google.common.collect.ImmutableSet; | ||
|
|
||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.regex.Pattern; | ||
|
|
||
| /** Resolves the incompatible cursor result-set behavior used by Connector/J releases. */ | ||
| public final class MysqlCursorFetchCompatibility { | ||
| private static final Set<String> MYSQL_CONNECTOR_J_CLIENT_NAMES = ImmutableSet.of( | ||
| "MySQL Connector/J", "MySQL Connector Java"); | ||
| private static final Pattern CONSUMES_METADATA_TERMINATOR = | ||
| Pattern.compile("^(?:(?:5|6|8)\\.|9\\.[0-4](?:\\.|$))"); | ||
| private static final Pattern VERSION = Pattern.compile("^\\d+(?:\\.\\d+)+(?:[-+].*)?$"); | ||
|
|
||
| public enum Behavior { | ||
| CONSUMES_METADATA_TERMINATOR, | ||
| STANDARD, | ||
| UNKNOWN | ||
| } | ||
|
|
||
| private MysqlCursorFetchCompatibility() { | ||
| } | ||
|
|
||
| public static Behavior resolve(Map<String, String> connectAttributes) { | ||
| String clientName = connectAttributes.get("_client_name"); | ||
| if (clientName == null) { | ||
| return Behavior.UNKNOWN; | ||
| } | ||
| if (!MYSQL_CONNECTOR_J_CLIENT_NAMES.contains(clientName)) { | ||
| return Behavior.STANDARD; | ||
| } | ||
|
|
||
| String clientVersion = connectAttributes.get("_client_version"); | ||
| if (clientVersion == null || !VERSION.matcher(clientVersion).matches()) { | ||
| return Behavior.UNKNOWN; | ||
| } | ||
| if (CONSUMES_METADATA_TERMINATOR.matcher(clientVersion).find()) { | ||
| return Behavior.CONSUMES_METADATA_TERMINATOR; | ||
| } | ||
| return Behavior.STANDARD; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -654,8 +654,23 @@ public void finalizeCommand() throws IOException { | |
| && ctx.getState().getStateType() != QueryState.MysqlStateType.ERR) { | ||
| ShowResultSet resultSet = executor.getShowResultSet(); | ||
| if (resultSet == null) { | ||
| executor.sendProxyQueryResult(); | ||
| packet = executor.getOutputPacket(); | ||
| if (ctx.getMysqlChannel().clientDeprecatedEOF() | ||
| && !executor.isForwardedClientDeprecatedEofApplied() | ||
| && executor.getProxyStatusCode() == 0) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Classify successful old-master reads without widening this guard A real forwarded SELECT cannot satisfy this gate: result producers finish with |
||
| if (executor.hasForwardedQueryResultPackets()) { | ||
| ctx.getState().setError(ErrorCode.ERR_NOT_SUPPORTED_YET, | ||
| "The master FE cannot preserve CLIENT_DEPRECATE_EOF while forwarding this query. " | ||
| + "Connect to the master FE or finish the FE rolling upgrade"); | ||
| } else { | ||
| // An old master has already completed a DDL/DML operation. Rebuild its final OK locally | ||
| // instead of returning an upgrade error that could make the client retry side effects. | ||
| ctx.getState().setOk(executor.getForwardedAffectedRows(), 0, null); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] Preserve the old master's complete OK result This rolling-upgrade branch rebuilds a successful DML response with only |
||
| } | ||
| packet = getResultPacket(); | ||
| } else { | ||
| executor.sendProxyQueryResult(); | ||
| packet = executor.getOutputPacket(); | ||
| } | ||
| } else { | ||
| executor.sendResultSet(resultSet); | ||
| packet = getResultPacket(); | ||
|
|
@@ -729,6 +744,8 @@ public TMasterOpResult proxyExecute(TMasterOpRequest request) throws TException | |
| if (request.isSetClientDeprecatedEOF() && request.isClientDeprecatedEOF()) { | ||
| ctx.getMysqlChannel().setClientDeprecatedEOF(); | ||
| } | ||
| ctx.setCursorFetchRequested(request.isSetCursorFetchRequested() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Handle cursor intent from old forwarding FEs During a rolling upgrade an old follower cannot set the new optional |
||
| && request.isCursorFetchRequested()); | ||
|
|
||
| ctx.setThreadLocalInfo(); | ||
| StmtExecutor executor = null; | ||
|
|
@@ -818,6 +835,7 @@ public TMasterOpResult proxyExecute(TMasterOpRequest request) throws TException | |
| ctx.getState().serverStatus |= MysqlServerStatusFlag.SERVER_MORE_RESULTS_EXISTS; | ||
| } | ||
| result.setPacket(getResultPacket()); | ||
| result.setClientDeprecatedEofApplied(ctx.getMysqlChannel().clientDeprecatedEOF()); | ||
| result.setStatus(ctx.getState().toString()); | ||
| if (ctx.getState().getStateType() == MysqlStateType.OK) { | ||
| result.setStatusCode(0); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -214,6 +214,7 @@ protected TMasterOpRequest buildStmtForwardParams() throws AnalysisException { | |
| if (null != ctx.getPrepareExecuteBuffer()) { | ||
| params.setPrepareExecuteBuffer(ctx.getPrepareExecuteBuffer()); | ||
| } | ||
| params.setCursorFetchRequested(ctx.isCursorFetchRequested()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Preserve binary execute mode without parameters For a zero-placeholder prepared SELECT, |
||
| } | ||
|
|
||
| ctx.getSessionContext().getDelegatedCredential().ifPresent((DelegatedCredential credential) -> { | ||
|
|
@@ -258,6 +259,20 @@ public ByteBuffer getOutputPacket() { | |
| return result.packet; | ||
| } | ||
|
|
||
| public boolean isClientDeprecatedEofApplied() { | ||
| return result != null && result.isSetClientDeprecatedEofApplied() | ||
| && result.isClientDeprecatedEofApplied(); | ||
| } | ||
|
|
||
| public boolean hasQueryResultPackets() { | ||
| return result != null && result.isSetQueryResultBufList() | ||
| && !result.getQueryResultBufList().isEmpty(); | ||
| } | ||
|
|
||
| public long getAffectedRows() { | ||
| return result != null && result.isSetAffectedRows() ? result.getAffectedRows() : 0; | ||
| } | ||
|
|
||
| public TUniqueId getQueryId() { | ||
| if (result != null && result.isSetQueryId()) { | ||
| return result.getQueryId(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| // Licensed to the Apache Software Foundation (ASF) under one | ||
| // or more contributor license agreements. See the NOTICE file | ||
| // distributed with this work for additional information | ||
| // regarding copyright ownership. The ASF licenses this file | ||
| // to you under the Apache License, Version 2.0 (the | ||
| // "License"); you may not use this file except in compliance | ||
| // with the License. You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, | ||
| // software distributed under the License is distributed on an | ||
| // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| // KIND, either express or implied. See the License for the | ||
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| package org.apache.doris.mysql; | ||
|
|
||
| import com.google.common.collect.ImmutableMap; | ||
| import org.junit.Assert; | ||
| import org.junit.Test; | ||
|
|
||
| import java.util.Collections; | ||
|
|
||
| public class MysqlCursorFetchCompatibilityTest { | ||
| @Test | ||
| public void testConnectorJBehaviorBoundaries() { | ||
| Assert.assertEquals(MysqlCursorFetchCompatibility.Behavior.CONSUMES_METADATA_TERMINATOR, | ||
| resolve("MySQL Connector Java", "5.1.49")); | ||
| Assert.assertEquals(MysqlCursorFetchCompatibility.Behavior.CONSUMES_METADATA_TERMINATOR, | ||
| resolve("MySQL Connector/J", "6.0.6")); | ||
| Assert.assertEquals(MysqlCursorFetchCompatibility.Behavior.CONSUMES_METADATA_TERMINATOR, | ||
| resolve("MySQL Connector/J", "8.2.0")); | ||
| Assert.assertEquals(MysqlCursorFetchCompatibility.Behavior.CONSUMES_METADATA_TERMINATOR, | ||
| resolve("MySQL Connector/J", "9.4.0")); | ||
| Assert.assertEquals(MysqlCursorFetchCompatibility.Behavior.STANDARD, | ||
| resolve("MySQL Connector/J", "9.5.0")); | ||
| Assert.assertEquals(MysqlCursorFetchCompatibility.Behavior.STANDARD, | ||
| resolve("MySQL Connector/J", "9.6.0")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testUnknownAndOtherClients() { | ||
| Assert.assertEquals(MysqlCursorFetchCompatibility.Behavior.UNKNOWN, | ||
| MysqlCursorFetchCompatibility.resolve(Collections.emptyMap())); | ||
| Assert.assertEquals(MysqlCursorFetchCompatibility.Behavior.UNKNOWN, | ||
| MysqlCursorFetchCompatibility.resolve(ImmutableMap.of("_client_name", "MySQL Connector/J"))); | ||
| Assert.assertEquals(MysqlCursorFetchCompatibility.Behavior.UNKNOWN, | ||
| resolve("MySQL Connector/J", "custom")); | ||
| Assert.assertEquals(MysqlCursorFetchCompatibility.Behavior.STANDARD, | ||
| resolve("MariaDB Connector/J", "3.5.6")); | ||
| } | ||
|
|
||
| private MysqlCursorFetchCompatibility.Behavior resolve(String clientName, String clientVersion) { | ||
| return MysqlCursorFetchCompatibility.resolve(ImmutableMap.of( | ||
| "_client_name", clientName, "_client_version", clientVersion)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] Read the negotiated EOF capability here
This condition is false in the new unit test, but it remains true for real legacy-EOF connections.
MysqlProto.negotiaterecords the client's bit only inMysqlChannel, then sets the serializer capability tocontext.getServerCapability(); that default mask always includesCLIENT_DEPRECATE_EOF.ProxyMysqlChannelstarts with the same default as well. Consequently an authenticated client that did not negotiate the flag still gets the trailing zero byte this change intends to remove. Please key this from the negotiated/channel capability, and propagate it to proxy serialization, or store the negotiated mask in the serializer, with a handshake-level test.