You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2019/12/27 09:43:09 UTC
[GitHub] [incubator-doris] infearOnTheWay opened a new pull request #2603:
Add nio support for mysql protocal implementation
infearOnTheWay opened a new pull request #2603: Add nio support for mysql protocal implementation
URL: https://github.com/apache/incubator-doris/pull/2603
#2602
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] infearOnTheWay commented on issue #2603: Add nio
support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
infearOnTheWay commented on issue #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#issuecomment-570495207
Since the idle check is still use connect scheduler, so *submit *is just
retained.
Sorry for too late reply for this mail, since I don't find the relative
conversation on my pr any more......
On Fri, Dec 27, 2019 at 10:47 PM Mingyu Chen <no...@github.com>
wrote:
> *@morningman* commented on this pull request.
> ------------------------------
>
> In fe/src/main/java/org/apache/doris/mysql/nio/AcceptListener.java
> <https://github.com/apache/incubator-doris/pull/2603#discussion_r361679254>
> :
>
> > +
> + public AcceptListener(ConnectScheduler connectScheduler) {
> + this.connectScheduler = connectScheduler;
> + }
> +
> + @Override
> + public void handleEvent(AcceptingChannel<StreamConnection> channel) {
> + try {
> + StreamConnection connection = channel.accept();
> + if (connection == null) {
> + return;
> + }
> + LOG.info("Connection established. remote={}", connection.getPeerAddress());
> + NConnectContext context = new NConnectContext(connection);
> + context.setCatalog(Catalog.getInstance());
> + connectScheduler.submit(context);
>
> Does connectScheduler.submit(context); still needed here?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/incubator-doris/pull/2603?email_source=notifications&email_token=AE5SSTZHNHERTO3SWR4NSILQ2YIRRA5CNFSM4J7ZIRM2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQJQD2Y#pullrequestreview-336789995>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AE5SSTZFFCG4IFPHZKMS4PDQ2YIRRANCNFSM4J7ZIRMQ>
> .
>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] infearOnTheWay commented on a change in pull
request #2603: Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
infearOnTheWay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361912121
##########
File path: fe/src/main/java/org/apache/doris/qe/QeService.java
##########
@@ -37,6 +37,21 @@ public QeService(int port) {
this.port = port;
}
+ public QeService(int port, boolean nioEnabled, ConnectScheduler scheduler) {
+ // Set up help module
+ try {
+ HelpModule.getInstance().setUpModule();
+ } catch (Exception e) {
+ LOG.error("Help module failed, because:", e);
+ }
+ this.port = port;
+ if (nioEnabled) {
+ mysqlServer = new NMysqlServer(port, scheduler);
+ } else {
+ mysqlServer = new MysqlServer(port, scheduler);
+ }
+ }
+
public QeService(int port, ConnectScheduler scheduler) {
Review comment:
OK
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2603: Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r362178093
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/NMysqlChannel.java
##########
@@ -0,0 +1,111 @@
+// 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.nio;
+
+import org.apache.doris.mysql.MysqlChannel;
+import org.apache.doris.qe.ConnectProcessor;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.xnio.StreamConnection;
+import org.xnio.channels.Channels;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.ByteBuffer;
+
+/**
+ * mysql Channel based on nio.
+ */
+public class NMysqlChannel extends MysqlChannel {
+ protected final Logger LOG = LogManager.getLogger(this.getClass());
+ private StreamConnection conn;
+
+ public NMysqlChannel(StreamConnection connection) {
+ super();
+ this.conn = connection;
+ if (connection.getPeerAddress() instanceof InetSocketAddress) {
+ InetSocketAddress address = (InetSocketAddress) connection.getPeerAddress();
+ remoteHostPortString = address.getHostString() + ":" + address.getPort();
+ remoteIp = address.getAddress().getHostAddress();
+ } else {
+ // Reach here, what's it?
+ remoteHostPortString = connection.getPeerAddress().toString();
+ remoteIp = connection.getPeerAddress().toString();
+ }
+ }
+
+ /**
+ * read packet until all data is ready, unless block.
+ *
+ * @param dstBuf
+ * @return
+ * @throws IOException
+ */
+ @Override
+ protected int readAll(ByteBuffer dstBuf) throws IOException {
+ int ret = Channels.readBlocking(conn.getSourceChannel(), dstBuf);
+ // return -1 when remote peer close the channel
+ if (ret == -1) {
+ return 0;
+ }
+ return ret;
Review comment:
There may be a problem here.
When the MySQL request message is large, it will be split into multiple TCP packets, but it is only read once here, and `readBlocking()` does not guarantee that the content of the parameter `dstBuf` is filled(see [jboss doc](https://docs.jboss.org/xnio/3.1/api/org/xnio/channels/Channels.html#readBlocking(C,%20java.nio.ByteBuffer))), so it is easy causes fewer data to be read. In other words, a complete MySQL logical packet can not be read, which will cause an error to occur in the upper layer.
For example, when the local TCP buffer is full, the remaining part of the logical packet will only be reached after reading the data that has arrived first, and then it can be read.
Here, the length we want to read should be `dstBuf.remaining()`. A simple solution to achieve this is to add a loop logic(like the existing implementation), to make sure this function will not return until enough data is read, but this may take a long time for the IO thread because it is not sure what time the remaining packets are will arrive. However, the problem is not big in practice.
A slightly more complicated but better solution is to try to read every time a readable event arrives(likes what you did now). If there is not a complete logical packet, then just record the data. Processing is not done until a logical packet has been read, but this may require slightly larger changes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] infearOnTheWay commented on a change in pull
request #2603: Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
infearOnTheWay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361921719
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/NMysqlServer.java
##########
@@ -0,0 +1,92 @@
+// 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.nio;
+
+import org.apache.doris.mysql.MysqlServer;
+import org.apache.doris.qe.ConnectScheduler;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.xnio.OptionMap;
+import org.xnio.Options;
+import org.xnio.StreamConnection;
+import org.xnio.Xnio;
+import org.xnio.XnioWorker;
+import org.xnio.channels.AcceptingChannel;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+
+/**
+ * nio version mysql protocol implementation based on XNIO。
+ *
+ * Created by infear on 2019/12/19.
Review comment:
OK
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2603:
Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361662196
##########
File path: fe/src/main/java/org/apache/doris/qe/ConnectContext.java
##########
@@ -112,6 +112,17 @@ public boolean isSend() {
return this.isSend;
}
+ public ConnectContext(){
Review comment:
```suggestion
public ConnectContext() {
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] infearOnTheWay commented on a change in pull
request #2603: Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
infearOnTheWay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361951114
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/ReadListener.java
##########
@@ -0,0 +1,66 @@
+// 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.nio;
+
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.ConnectProcessor;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.xnio.ChannelListener;
+import org.xnio.XnioIoThread;
+import org.xnio.conduits.ConduitStreamSourceChannel;
+
+/**
+ * Created by infear on 2019/12/20.
+ */
+public class ReadListener implements ChannelListener<ConduitStreamSourceChannel> {
+ private final Logger LOG = LogManager.getLogger(this.getClass());
+ private NConnectContext ctx;
+ private ConnectProcessor connectProcessor;
+
+ public ReadListener(NConnectContext nConnectContext, ConnectProcessor connectProcessor) {
+ this.ctx = nConnectContext;
+ this.connectProcessor = connectProcessor;
+ }
+
+ @Override
+ public void handleEvent(ConduitStreamSourceChannel channel) {
+ // suspend must be call sync in current thread (the IO-Thread notify the read event),
+ // otherwise multi handler(task thread) would be waked up by once query.
+ XnioIoThread.requireCurrentThread();
+ ctx.suspendAcceptQuery();
+ // start async query handle in task thread.
+ channel.getWorker().execute(() -> {
Review comment:
@lingbin @imay
After discussion with @imay, according to the case in doris, it woud be better to use a cahceThreadPool instead of fixed-size(16) thread pool mentioned above. I would optimize this part and also try to make users be able to inject customized thread pool. It should be more flexible.
Thanks for your advice!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a change in pull request
#2603: Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361679254
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/AcceptListener.java
##########
@@ -0,0 +1,85 @@
+// 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.nio;
+
+import org.apache.doris.catalog.Catalog;
+import org.apache.doris.mysql.MysqlProto;
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.ConnectProcessor;
+import org.apache.doris.qe.ConnectScheduler;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.xnio.ChannelListener;
+import org.xnio.StreamConnection;
+import org.xnio.channels.AcceptingChannel;
+
+import java.io.IOException;
+
+/**
+ * Created by infear on 2019/12/19.
+ */
+public class AcceptListener implements ChannelListener<AcceptingChannel<StreamConnection>> {
+ private final Logger LOG = LogManager.getLogger(this.getClass());
+ private ConnectScheduler connectScheduler;
+
+ public AcceptListener(ConnectScheduler connectScheduler) {
+ this.connectScheduler = connectScheduler;
+ }
+
+ @Override
+ public void handleEvent(AcceptingChannel<StreamConnection> channel) {
+ try {
+ StreamConnection connection = channel.accept();
+ if (connection == null) {
+ return;
+ }
+ LOG.info("Connection established. remote={}", connection.getPeerAddress());
+ NConnectContext context = new NConnectContext(connection);
+ context.setCatalog(Catalog.getInstance());
+ connectScheduler.submit(context);
Review comment:
Does `connectScheduler.submit(context);` still needed here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2603:
Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361662435
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/NMysqlServer.java
##########
@@ -0,0 +1,92 @@
+// 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.nio;
+
+import org.apache.doris.mysql.MysqlServer;
+import org.apache.doris.qe.ConnectScheduler;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.xnio.OptionMap;
+import org.xnio.Options;
+import org.xnio.StreamConnection;
+import org.xnio.Xnio;
+import org.xnio.XnioWorker;
+import org.xnio.channels.AcceptingChannel;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+
+/**
+ * nio version mysql protocol implementation based on XNIO。
+ *
+ * Created by infear on 2019/12/19.
Review comment:
better to remove this author information
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2603:
Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361662072
##########
File path: fe/src/main/java/org/apache/doris/qe/QeService.java
##########
@@ -37,6 +37,21 @@ public QeService(int port) {
this.port = port;
}
+ public QeService(int port, boolean nioEnabled, ConnectScheduler scheduler) {
+ // Set up help module
+ try {
+ HelpModule.getInstance().setUpModule();
+ } catch (Exception e) {
+ LOG.error("Help module failed, because:", e);
+ }
+ this.port = port;
+ if (nioEnabled) {
+ mysqlServer = new NMysqlServer(port, scheduler);
+ } else {
+ mysqlServer = new MysqlServer(port, scheduler);
+ }
+ }
+
public QeService(int port, ConnectScheduler scheduler) {
Review comment:
If this is not used anymore. Better to remove it
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] infearOnTheWay commented on a change in pull
request #2603: Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
infearOnTheWay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361934964
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/NMysqlServer.java
##########
@@ -0,0 +1,92 @@
+// 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.nio;
+
+import org.apache.doris.mysql.MysqlServer;
+import org.apache.doris.qe.ConnectScheduler;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.xnio.OptionMap;
+import org.xnio.Options;
+import org.xnio.StreamConnection;
+import org.xnio.Xnio;
+import org.xnio.XnioWorker;
+import org.xnio.channels.AcceptingChannel;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+
+/**
+ * nio version mysql protocol implementation based on XNIO。
+ *
+ * Created by infear on 2019/12/19.
+ */
+public class NMysqlServer extends MysqlServer{
+ private final Logger LOG = LogManager.getLogger(this.getClass());
+
+ private XnioWorker xnioWorker;
+
+ private AcceptListener acceptListener;
+
+ private AcceptingChannel<StreamConnection> server;
+
+ public NMysqlServer(int port, ConnectScheduler connectScheduler) {
+ this.port = port;
+ try {
+ this.xnioWorker = Xnio.getInstance().createWorker(OptionMap.builder()
+ .set(Options.WORKER_IO_THREADS, 4)
+ .set(Options.WORKER_TASK_MAX_THREADS, 16)
Review comment:
> Better to make these options can be configured.
OK, I would make all nio options be configured
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] infearOnTheWay commented on a change in pull
request #2603: Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
infearOnTheWay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361933507
##########
File path: fe/src/main/java/org/apache/doris/common/Config.java
##########
@@ -266,6 +266,11 @@
*/
@ConfField public static int query_port = 9030;
+ /*
+ * nio enabled
+ */
+ @ConfField public static boolean nio_enabled = false;
Review comment:
NP
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2603:
Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361664423
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/NMysqlServer.java
##########
@@ -0,0 +1,92 @@
+// 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.nio;
+
+import org.apache.doris.mysql.MysqlServer;
+import org.apache.doris.qe.ConnectScheduler;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.xnio.OptionMap;
+import org.xnio.Options;
+import org.xnio.StreamConnection;
+import org.xnio.Xnio;
+import org.xnio.XnioWorker;
+import org.xnio.channels.AcceptingChannel;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+
+/**
+ * nio version mysql protocol implementation based on XNIO。
+ *
+ * Created by infear on 2019/12/19.
+ */
+public class NMysqlServer extends MysqlServer{
+ private final Logger LOG = LogManager.getLogger(this.getClass());
+
+ private XnioWorker xnioWorker;
+
+ private AcceptListener acceptListener;
+
+ private AcceptingChannel<StreamConnection> server;
+
+ public NMysqlServer(int port, ConnectScheduler connectScheduler) {
+ this.port = port;
+ try {
+ this.xnioWorker = Xnio.getInstance().createWorker(OptionMap.builder()
+ .set(Options.WORKER_IO_THREADS, 4)
+ .set(Options.WORKER_TASK_MAX_THREADS, 16)
Review comment:
Better to make these options can be configured.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay merged pull request #2603: Add nio support
for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
imay merged pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] infearOnTheWay commented on a change in pull
request #2603: Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
infearOnTheWay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r362385781
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/NMysqlChannel.java
##########
@@ -0,0 +1,111 @@
+// 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.nio;
+
+import org.apache.doris.mysql.MysqlChannel;
+import org.apache.doris.qe.ConnectProcessor;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.xnio.StreamConnection;
+import org.xnio.channels.Channels;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.ByteBuffer;
+
+/**
+ * mysql Channel based on nio.
+ */
+public class NMysqlChannel extends MysqlChannel {
+ protected final Logger LOG = LogManager.getLogger(this.getClass());
+ private StreamConnection conn;
+
+ public NMysqlChannel(StreamConnection connection) {
+ super();
+ this.conn = connection;
+ if (connection.getPeerAddress() instanceof InetSocketAddress) {
+ InetSocketAddress address = (InetSocketAddress) connection.getPeerAddress();
+ remoteHostPortString = address.getHostString() + ":" + address.getPort();
+ remoteIp = address.getAddress().getHostAddress();
+ } else {
+ // Reach here, what's it?
+ remoteHostPortString = connection.getPeerAddress().toString();
+ remoteIp = connection.getPeerAddress().toString();
+ }
+ }
+
+ /**
+ * read packet until all data is ready, unless block.
+ *
+ * @param dstBuf
+ * @return
+ * @throws IOException
+ */
+ @Override
+ protected int readAll(ByteBuffer dstBuf) throws IOException {
+ int ret = Channels.readBlocking(conn.getSourceChannel(), dstBuf);
+ // return -1 when remote peer close the channel
+ if (ret == -1) {
+ return 0;
+ }
+ return ret;
Review comment:
@lingbin
> A simple solution to achieve this is to add a loop logic(like the existing implementation), to make sure this function will not return until enough data is read,but this may take a long time for the IO thread because it is not sure what time the remaining packets are will arrive.
The loop logic is currentlt retained in `MysqlChannel.fetchOnePacket`, and only the method `readAll` is overwritten based on nio. So if one logic packets is split to multiple TCP packets, all packets would be read and compose.
On the other hand, this read process is handled by **task thread** instead of **io thread**(io thread just notify readlistenter while read event arrived). So it would not matter.
> When the MySQL request message is large, it will be split into multiple TCP packets, but it is only read once here, and readBlocking() does not guarantee that the content of the parameter dstBuf is filled
The source code of `readBlocking` is as belows:
```
public static <C extends ReadableByteChannel & SuspendableReadChannel> int readBlocking(C channel, ByteBuffer buffer) throws IOException {
int res;
while ((res = channel.read(buffer)) == 0 && buffer.hasRemaining()) {
channel.awaitReadable();
}
return res;
}
```
It seems that `dstbuffer` would be filled, unless one TCP packet is split to multiple TCP packets. In other words, **different parts of one TCP packets** do not arrived at same time.
Then just part is read and cause error in upper layer. Is it this the problem you concerned about?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a change in pull request
#2603: Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361679254
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/AcceptListener.java
##########
@@ -0,0 +1,85 @@
+// 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.nio;
+
+import org.apache.doris.catalog.Catalog;
+import org.apache.doris.mysql.MysqlProto;
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.ConnectProcessor;
+import org.apache.doris.qe.ConnectScheduler;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.xnio.ChannelListener;
+import org.xnio.StreamConnection;
+import org.xnio.channels.AcceptingChannel;
+
+import java.io.IOException;
+
+/**
+ * Created by infear on 2019/12/19.
+ */
+public class AcceptListener implements ChannelListener<AcceptingChannel<StreamConnection>> {
+ private final Logger LOG = LogManager.getLogger(this.getClass());
+ private ConnectScheduler connectScheduler;
+
+ public AcceptListener(ConnectScheduler connectScheduler) {
+ this.connectScheduler = connectScheduler;
+ }
+
+ @Override
+ public void handleEvent(AcceptingChannel<StreamConnection> channel) {
+ try {
+ StreamConnection connection = channel.accept();
+ if (connection == null) {
+ return;
+ }
+ LOG.info("Connection established. remote={}", connection.getPeerAddress());
+ NConnectContext context = new NConnectContext(connection);
+ context.setCatalog(Catalog.getInstance());
+ connectScheduler.submit(context);
Review comment:
Does `connectScheduler.submit(context);` still needed here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2603:
Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361664318
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/NMysqlServer.java
##########
@@ -0,0 +1,92 @@
+// 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.nio;
+
+import org.apache.doris.mysql.MysqlServer;
+import org.apache.doris.qe.ConnectScheduler;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.xnio.OptionMap;
+import org.xnio.Options;
+import org.xnio.StreamConnection;
+import org.xnio.Xnio;
+import org.xnio.XnioWorker;
+import org.xnio.channels.AcceptingChannel;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+
+/**
+ * nio version mysql protocol implementation based on XNIO。
+ *
+ * Created by infear on 2019/12/19.
+ */
+public class NMysqlServer extends MysqlServer{
+ private final Logger LOG = LogManager.getLogger(this.getClass());
+
+ private XnioWorker xnioWorker;
+
+ private AcceptListener acceptListener;
+
+ private AcceptingChannel<StreamConnection> server;
+
+ public NMysqlServer(int port, ConnectScheduler connectScheduler) {
+ this.port = port;
+ try {
+ this.xnioWorker = Xnio.getInstance().createWorker(OptionMap.builder()
+ .set(Options.WORKER_IO_THREADS, 4)
+ .set(Options.WORKER_TASK_MAX_THREADS, 16)
+ .set(Options.WORKER_NAME, "palo-mysql-nio")
Review comment:
```suggestion
.set(Options.WORKER_NAME, "doris-mysql-nio")
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2603: Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361673107
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/ReadListener.java
##########
@@ -0,0 +1,66 @@
+// 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.nio;
+
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.ConnectProcessor;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.xnio.ChannelListener;
+import org.xnio.XnioIoThread;
+import org.xnio.conduits.ConduitStreamSourceChannel;
+
+/**
+ * Created by infear on 2019/12/20.
+ */
+public class ReadListener implements ChannelListener<ConduitStreamSourceChannel> {
+ private final Logger LOG = LogManager.getLogger(this.getClass());
+ private NConnectContext ctx;
+ private ConnectProcessor connectProcessor;
+
+ public ReadListener(NConnectContext nConnectContext, ConnectProcessor connectProcessor) {
+ this.ctx = nConnectContext;
+ this.connectProcessor = connectProcessor;
+ }
+
+ @Override
+ public void handleEvent(ConduitStreamSourceChannel channel) {
+ // suspend must be call sync in current thread (the IO-Thread notify the read event),
+ // otherwise multi handler(task thread) would be waked up by once query.
+ XnioIoThread.requireCurrentThread();
+ ctx.suspendAcceptQuery();
+ // start async query handle in task thread.
+ channel.getWorker().execute(() -> {
Review comment:
I don't know much about the use of `jboss.xnio` library, just read its documentation.
I have a question here: will this run in `WORKER THREAD pool`, which was configured to 16 in `NMysqlServer`(use `Options.WORKER_TASK_MAX_THREADS`)?
If it is, there may be a problem. For example: if we have 16 slow queries, such as running for 5 minutes, which makes these 16 threads are filled. And because there are no threads available for subsequent query requests, the server will hang for 5 minutes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2603:
Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361663840
##########
File path: fe/src/main/java/org/apache/doris/mysql/MysqlChannel.java
##########
@@ -33,25 +33,34 @@
public class MysqlChannel {
// max length which one MySQL physical can hold, if one logical packet is bigger than this,
// one packet will split to many packets
- private static final int MAX_PHYSICAL_PACKET_LENGTH = 0xffffff - 1;
+ protected static final int MAX_PHYSICAL_PACKET_LENGTH = 0xffffff - 1;
// MySQL packet header length
- private static final int PACKET_HEADER_LEN = 4;
+ protected static final int PACKET_HEADER_LEN = 4;
// logger for this class
- private static final Logger LOG = LogManager.getLogger(MysqlChannel.class);
+ protected static final Logger LOG = LogManager.getLogger(MysqlChannel.class);
// next sequence id to receive or send
- private int sequenceId;
+ protected int sequenceId;
// channel connected with client
- private SocketChannel channel;
+ protected SocketChannel channel;
// used to receive/send header, avoiding new this many time.
- private ByteBuffer headerByteBuffer = ByteBuffer.allocate(PACKET_HEADER_LEN);
+ protected ByteBuffer headerByteBuffer = ByteBuffer.allocate(PACKET_HEADER_LEN);
// default packet byte buffer for most packet
- private ByteBuffer defaultBuffer = ByteBuffer.allocate(16 * 1024);
- private ByteBuffer sendBuffer;
+ protected ByteBuffer defaultBuffer = ByteBuffer.allocate(16 * 1024);
+ protected ByteBuffer sendBuffer;
// for log and show
- private String remoteHostPortString;
- private String remoteIp;
- private boolean isSend;
+ protected String remoteHostPortString;
+ protected String remoteIp;
+ protected boolean isSend;
+
+ public MysqlChannel() {
Review comment:
Is protected enough?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2603:
Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361663695
##########
File path: fe/src/main/java/org/apache/doris/common/Config.java
##########
@@ -266,6 +266,11 @@
*/
@ConfField public static int query_port = 9030;
+ /*
+ * nio enabled
+ */
+ @ConfField public static boolean nio_enabled = false;
Review comment:
Doris has many network interface, this is only for mysql interface. Better to rename it to "mysql_service_nio?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2603:
Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361675039
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/ReadListener.java
##########
@@ -0,0 +1,66 @@
+// 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.nio;
+
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.ConnectProcessor;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.xnio.ChannelListener;
+import org.xnio.XnioIoThread;
+import org.xnio.conduits.ConduitStreamSourceChannel;
+
+/**
+ * Created by infear on 2019/12/20.
+ */
+public class ReadListener implements ChannelListener<ConduitStreamSourceChannel> {
+ private final Logger LOG = LogManager.getLogger(this.getClass());
+ private NConnectContext ctx;
+ private ConnectProcessor connectProcessor;
+
+ public ReadListener(NConnectContext nConnectContext, ConnectProcessor connectProcessor) {
+ this.ctx = nConnectContext;
+ this.connectProcessor = connectProcessor;
+ }
+
+ @Override
+ public void handleEvent(ConduitStreamSourceChannel channel) {
+ // suspend must be call sync in current thread (the IO-Thread notify the read event),
+ // otherwise multi handler(task thread) would be waked up by once query.
+ XnioIoThread.requireCurrentThread();
+ ctx.suspendAcceptQuery();
+ // start async query handle in task thread.
+ channel.getWorker().execute(() -> {
Review comment:
I have the same question of @lingbin . Because it's common that a query last for some minutes. If the worker pool size is static, it will cause other query can't be processed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2603:
Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361662581
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/NConnectContext.java
##########
@@ -0,0 +1,62 @@
+// 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.nio;
+
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.ConnectProcessor;
+import org.xnio.StreamConnection;
+
+import java.io.IOException;
+
+/**
+ * Created by infear on 2019/12/19.
+ */
+public class NConnectContext extends ConnectContext{
Review comment:
```suggestion
public class NConnectContext extends ConnectContext {
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #2603:
Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361662468
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/NMysqlServer.java
##########
@@ -0,0 +1,92 @@
+// 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.nio;
+
+import org.apache.doris.mysql.MysqlServer;
+import org.apache.doris.qe.ConnectScheduler;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.xnio.OptionMap;
+import org.xnio.Options;
+import org.xnio.StreamConnection;
+import org.xnio.Xnio;
+import org.xnio.XnioWorker;
+import org.xnio.channels.AcceptingChannel;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+
+/**
+ * nio version mysql protocol implementation based on XNIO。
+ *
+ * Created by infear on 2019/12/19.
+ */
+public class NMysqlServer extends MysqlServer{
Review comment:
```suggestion
public class NMysqlServer extends MysqlServer {
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] infearOnTheWay commented on a change in pull
request #2603: Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
infearOnTheWay commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r361922024
##########
File path: fe/src/main/java/org/apache/doris/mysql/MysqlChannel.java
##########
@@ -33,25 +33,34 @@
public class MysqlChannel {
// max length which one MySQL physical can hold, if one logical packet is bigger than this,
// one packet will split to many packets
- private static final int MAX_PHYSICAL_PACKET_LENGTH = 0xffffff - 1;
+ protected static final int MAX_PHYSICAL_PACKET_LENGTH = 0xffffff - 1;
// MySQL packet header length
- private static final int PACKET_HEADER_LEN = 4;
+ protected static final int PACKET_HEADER_LEN = 4;
// logger for this class
- private static final Logger LOG = LogManager.getLogger(MysqlChannel.class);
+ protected static final Logger LOG = LogManager.getLogger(MysqlChannel.class);
// next sequence id to receive or send
- private int sequenceId;
+ protected int sequenceId;
// channel connected with client
- private SocketChannel channel;
+ protected SocketChannel channel;
// used to receive/send header, avoiding new this many time.
- private ByteBuffer headerByteBuffer = ByteBuffer.allocate(PACKET_HEADER_LEN);
+ protected ByteBuffer headerByteBuffer = ByteBuffer.allocate(PACKET_HEADER_LEN);
// default packet byte buffer for most packet
- private ByteBuffer defaultBuffer = ByteBuffer.allocate(16 * 1024);
- private ByteBuffer sendBuffer;
+ protected ByteBuffer defaultBuffer = ByteBuffer.allocate(16 * 1024);
+ protected ByteBuffer sendBuffer;
// for log and show
- private String remoteHostPortString;
- private String remoteIp;
- private boolean isSend;
+ protected String remoteHostPortString;
+ protected String remoteIp;
+ protected boolean isSend;
+
+ public MysqlChannel() {
Review comment:
yes @imay
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2603: Add nio support for mysql protocol implementation
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2603: Add nio support for mysql protocol implementation
URL: https://github.com/apache/incubator-doris/pull/2603#discussion_r363147726
##########
File path: fe/src/main/java/org/apache/doris/mysql/nio/NMysqlChannel.java
##########
@@ -0,0 +1,111 @@
+// 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.nio;
+
+import org.apache.doris.mysql.MysqlChannel;
+import org.apache.doris.qe.ConnectProcessor;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.xnio.StreamConnection;
+import org.xnio.channels.Channels;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.ByteBuffer;
+
+/**
+ * mysql Channel based on nio.
+ */
+public class NMysqlChannel extends MysqlChannel {
+ protected final Logger LOG = LogManager.getLogger(this.getClass());
+ private StreamConnection conn;
+
+ public NMysqlChannel(StreamConnection connection) {
+ super();
+ this.conn = connection;
+ if (connection.getPeerAddress() instanceof InetSocketAddress) {
+ InetSocketAddress address = (InetSocketAddress) connection.getPeerAddress();
+ remoteHostPortString = address.getHostString() + ":" + address.getPort();
+ remoteIp = address.getAddress().getHostAddress();
+ } else {
+ // Reach here, what's it?
+ remoteHostPortString = connection.getPeerAddress().toString();
+ remoteIp = connection.getPeerAddress().toString();
+ }
+ }
+
+ /**
+ * read packet until all data is ready, unless block.
+ *
+ * @param dstBuf
+ * @return
+ * @throws IOException
+ */
+ @Override
+ protected int readAll(ByteBuffer dstBuf) throws IOException {
+ int ret = Channels.readBlocking(conn.getSourceChannel(), dstBuf);
+ // return -1 when remote peer close the channel
+ if (ret == -1) {
+ return 0;
+ }
+ return ret;
Review comment:
**The problem has been discussed again offline, and here is a summary.**
According to the current implementation logic, when a MySQL message is relatively large, we must ensure that `readAll()` fills `desBuf`, and `readBlocking()` in `jboss.xnio` can not achieve this. So for the sake of simplicity, a loop logic will be added into `readAll()` to ensure that desBuf is filled (that is, the first method mentioned above)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org