kenhuuu commented on code in PR #3466:
URL: https://github.com/apache/tinkerpop/pull/3466#discussion_r3443646441
##########
docs/src/reference/gremlin-variants.asciidoc:
##########
@@ -991,24 +991,27 @@ The following table describes the various configuration
options for the Gremlin
|auth.password |The password to submit on requests that require basic
authentication. |_none_
|auth.region |The region setting for sigv4 authentication. |_none_
|auth.serviceName |The service name setting for sigv4 authentication. |_none_
-|connectionPool.connectionSetupTimeoutMillis | Duration of time in
milliseconds provided for connection setup to complete which includes the SSL
handshake. |15000
+|connectionPool.compression |The wire compression algorithm negotiated with
the server. Can be: `NONE` or `DEFLATE`. |DEFLATE
+|connectionPool.connectTimeout | Duration of time in milliseconds that bounds
TCP connection establishment (transport setup, including the SSL handshake).
Formerly `connectionSetupTimeoutMillis`, which is still accepted as a
deprecated alias. |5000
|connectionPool.enableSsl |Determines if SSL should be enabled or not. If
enabled on the server then it must be enabled on the client. |false
-|connectionPool.idleConnectionTimeout | Duration of time in milliseconds that
the driver will allow a channel to not receive read or writes before it
automatically closes. |180000
+|connectionPool.idleTimeout | Duration of time in milliseconds that the driver
will allow a channel to not receive read or writes before it automatically
closes. Formerly `idleConnectionTimeoutMillis`, which is still accepted as a
deprecated alias. |180000
Review Comment:
This is something I should probably have mentioned in the devlist and not
here but sometimes its nice to just have the Ms post-fix so something like
"idleTimeoutMs" so its immediately obvious what the unit is.
##########
docs/src/reference/gremlin-variants.asciidoc:
##########
@@ -991,24 +991,27 @@ The following table describes the various configuration
options for the Gremlin
|auth.password |The password to submit on requests that require basic
authentication. |_none_
|auth.region |The region setting for sigv4 authentication. |_none_
|auth.serviceName |The service name setting for sigv4 authentication. |_none_
-|connectionPool.connectionSetupTimeoutMillis | Duration of time in
milliseconds provided for connection setup to complete which includes the SSL
handshake. |15000
+|connectionPool.compression |The wire compression algorithm negotiated with
the server. Can be: `NONE` or `DEFLATE`. |DEFLATE
+|connectionPool.connectTimeout | Duration of time in milliseconds that bounds
TCP connection establishment (transport setup, including the SSL handshake).
Formerly `connectionSetupTimeoutMillis`, which is still accepted as a
deprecated alias. |5000
|connectionPool.enableSsl |Determines if SSL should be enabled or not. If
enabled on the server then it must be enabled on the client. |false
-|connectionPool.idleConnectionTimeout | Duration of time in milliseconds that
the driver will allow a channel to not receive read or writes before it
automatically closes. |180000
+|connectionPool.idleTimeout | Duration of time in milliseconds that the driver
will allow a channel to not receive read or writes before it automatically
closes. Formerly `idleConnectionTimeoutMillis`, which is still accepted as a
deprecated alias. |180000
+|connectionPool.keepAliveTime | Idle time in milliseconds before TCP
keep-alive probes begin on an otherwise idle connection. Enables `SO_KEEPALIVE`
on the socket and, where supported (JDK 11+ on Linux/macOS via `TCP_KEEPIDLE`),
sets the per-socket idle time. Set to `0` to disable. |30000
|connectionPool.keyStore |The private key in JKS or PKCS#12 format. |_none_
|connectionPool.keyStorePassword |The password of the `keyStore` if it is
password-protected. |_none_
|connectionPool.keyStoreType |`PKCS12` |_none_
-|connectionPool.maxResponseContentLength |The maximum length in bytes that a
message can be received from the server. |2147483647
-|connectionPool.maxSize |The maximum size of a connection pool for a host. |128
-|connectionPool.maxWaitForConnection |The amount of time in milliseconds to
wait for a new connection before timing out. |3000
+|connectionPool.maxConnections |The maximum size of a connection pool for a
host. Formerly `maxConnectionPoolSize`, which is still accepted as a deprecated
alias. |128
+|connectionPool.maxResponseHeaderBytes |The maximum size in bytes allowed for
the HTTP response headers. |8192
+|connectionPool.maxWaitForConnection |The amount of time in milliseconds to
wait for a new connection before timing out. |16000
|connectionPool.maxWaitForClose |The amount of time in milliseconds to wait
for pending messages to be returned from the server before closing the
connection. |3000
|connectionPool.reconnectInterval |The amount of time in milliseconds to wait
before trying to reconnect to a dead host. |1000
-|connectionPool.resultIterationBatchSize |The override value for the size of
the result batches to be returned from the server. |64
+|connectionPool.defaultBatchSize |The default value for the per-request batch
size used when a request does not specify one. Formerly
`resultIterationBatchSize`, which is still accepted as a deprecated alias. |64
Review Comment:
Again, probably devlist thing, but why does this have the word "default" in
it?
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java:
##########
@@ -136,6 +161,59 @@ public Connection(final URI uri, final ConnectionPool
pool) throws ConnectionExc
}
}
+ /**
+ * Configures the connection establishment timeout on the supplied {@link
Bootstrap}. The {@code connectTimeout}
+ * is expressed in milliseconds and bounds the TCP connection setup
(including the SSL handshake) performed by
+ * {@code b.connect()}. It is applied via Netty's {@link
ChannelOption#CONNECT_TIMEOUT_MILLIS}.
+ */
+ static void configureConnectTimeout(final Bootstrap b, final long
connectTimeout) {
+ b.option(ChannelOption.CONNECT_TIMEOUT_MILLIS, (int) connectTimeout);
Review Comment:
If Netty can only take an int, then connectTimeout should also be an int to
prevent silent truncation.
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Channelizer.java:
##########
@@ -139,6 +142,17 @@ public void init(final Connection connection) {
@Override
protected void initChannel(final SocketChannel socketChannel) {
final ChannelPipeline pipeline = socketChannel.pipeline();
+
+ // The proxy handler must run before the SSL handler so the
CONNECT tunnel is established prior to the
+ // TLS handshake.
+ final ProxyOptions proxy = cluster.getProxy();
Review Comment:
This doesn't need to be done for this PR, but a Jira would be nice to add
proxy testing to the SimpleTestServer thing in
gremlin-tools/gremlin-socket-server
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/handler/ReadTimeoutHandler.java:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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.tinkerpop.gremlin.driver.handler;
+
+import io.netty.channel.ChannelDuplexHandler;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelPromise;
+import io.netty.handler.codec.http.LastHttpContent;
+import io.netty.handler.timeout.ReadTimeoutException;
+import io.netty.util.concurrent.ScheduledFuture;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Bounds the idle time between inbound response chunks on a per-request
basis. The timeout is <em>armed</em> when a
+ * request is written and rescheduled on every inbound read; it is
<em>disarmed</em> once the last chunk of the
+ * response arrives. This differs from a static {@link
io.netty.handler.timeout.ReadTimeoutHandler} so that it never
+ * fires while a pooled connection sits idle between requests (which is the
concern of the idle-pool-close handler).
+ * <p>
+ * When the timeout elapses while awaiting a response a {@link
ReadTimeoutException} is fired up the pipeline and the
+ * channel is closed.
+ */
+public class ReadTimeoutHandler extends ChannelDuplexHandler {
Review Comment:
This is not what I expected when I saw this. This is an idle read timeout
but not a total read timeout which is what I would have expected. This overlaps
with `idleTimeout`.
##########
docs/src/upgrade/release-4.x.x.asciidoc:
##########
@@ -32,6 +32,54 @@ complete list of all the modifications that are part of this
release.
=== Upgrading for Users
+==== Standardizing Java Connection Options
+
+TinkerPop 4.x standardizes connection option names and defaults across the
GLVs. In the Java reference driver
+(`gremlin-driver`), several `Cluster.Builder` options have been renamed for
consistency, with the old names retained
+as deprecated aliases so that existing code keeps compiling, and a number of
new options have been added. The notes
Review Comment:
For this major version change, I suggest we reduce the clutter and simply
delete the old names.
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/RequestOptions.java:
##########
@@ -150,9 +150,20 @@ public static Builder from(final RequestOptions options) {
return builder;
}
+ /**
+ * Sets the traversal source (or graph) alias to rebind to "g" on the
request.
+ */
+ public Builder traversalSource(final String traversalSource) {
Review Comment:
Nit: I think this has traditionally been referred to as the traversal source
name not just traversal source. Also the name "addG" was that because the field
in the actual request is just g. So it might even be better to just leave it as
"addG" although I also don't really like that name.
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverIntegrateTest.java:
##########
@@ -412,29 +412,6 @@ public void shouldProcessEvalInterruption() {
}
}
- @Test
- public void shouldEventuallySucceedAfterChannelLevelError() {
Review Comment:
This is a real test that is using the old max content length as a trigger. I
don't think this test should be deleted, just updated.
--
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]
