-
Notifications
You must be signed in to change notification settings - Fork 26
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
Feature/http2 #32
base: main
Are you sure you want to change the base?
Feature/http2 #32
Changes from 8 commits
739762d
1cff196
5ddaf98
4e39324
9f467f4
1a271f1
a1cb224
eaf3f3c
b067963
457d288
740812c
80d2523
7495015
788cb1e
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 |
---|---|---|
|
@@ -21,6 +21,7 @@ | |
|
||
import oracle.nosql.driver.Region.RegionProvider; | ||
import oracle.nosql.driver.iam.SignatureProvider; | ||
import io.netty.handler.ssl.ApplicationProtocolNames; | ||
import io.netty.handler.ssl.SslContext; | ||
|
||
/** | ||
|
@@ -142,6 +143,13 @@ public class NoSQLHandleConfig implements Cloneable { | |
*/ | ||
private int maxChunkSize = 0; | ||
|
||
/** | ||
* Default http protocols | ||
* | ||
* Default: prefer H2 but fallback to Http1.1 | ||
*/ | ||
private List<String> httpProtocols = new ArrayList<>(Arrays.asList(ApplicationProtocolNames.HTTP_2, ApplicationProtocolNames.HTTP_1_1)); | ||
|
||
/** | ||
* A RetryHandler, or null if not configured by the user. | ||
*/ | ||
|
@@ -553,6 +561,23 @@ public int getDefaultRequestTimeout() { | |
return timeout == 0 ? DEFAULT_TIMEOUT : timeout; | ||
} | ||
|
||
/** | ||
* | ||
* @return Http protocol settings | ||
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. needs an actual explanation of what's being returned |
||
*/ | ||
public List<String> getHttpProtocols() { | ||
return httpProtocols; | ||
} | ||
|
||
/** | ||
* Check if "h2" is in the protocols list | ||
* | ||
* @return true if "h2" is in the protocols list | ||
*/ | ||
public boolean useHttp2() { | ||
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. I was thinking that the list would be ordered by preference. While http1.1, http2 doesn't necessarily make sense that would be the logic. Also it doesn't seem that this method should be available to the public. Minimally it should be hidden. Or, just remove it (preferable) and do the logic in the lower level code 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. The list is order irrelevant. |
||
return this.httpProtocols.contains(ApplicationProtocolNames.HTTP_2); | ||
} | ||
|
||
/** | ||
* Returns the configured table request timeout value, in milliseconds. | ||
* The table request timeout default can be specified independently to allow | ||
|
@@ -631,6 +656,14 @@ public NoSQLHandleConfig setRequestTimeout(int timeout) { | |
return this; | ||
} | ||
|
||
public NoSQLHandleConfig setHttpProtocols(String ... protocols) { | ||
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. This needs javadoc comment |
||
this.httpProtocols = new ArrayList<>(2); | ||
for (String p : protocols) { | ||
this.httpProtocols.add(p); | ||
} | ||
return this; | ||
} | ||
|
||
/** | ||
* Sets the default table request timeout. | ||
* The table request timeout can be specified independently | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,9 @@ | |
import static oracle.nosql.driver.util.LogUtil.logWarning; | ||
|
||
import java.io.IOException; | ||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.List; | ||
import java.util.concurrent.ExecutionException; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.concurrent.TimeoutException; | ||
|
@@ -31,6 +34,7 @@ | |
import io.netty.channel.socket.nio.NioSocketChannel; | ||
import io.netty.handler.codec.http.DefaultFullHttpRequest; | ||
import io.netty.handler.codec.http.HttpRequest; | ||
import io.netty.handler.ssl.ApplicationProtocolNames; | ||
import io.netty.handler.ssl.SslContext; | ||
import io.netty.util.AttributeKey; | ||
import io.netty.util.concurrent.Future; | ||
|
@@ -96,6 +100,8 @@ public class HttpClient { | |
private final String host; | ||
private final int port; | ||
private final String name; | ||
private final List<String> httpProtocols; | ||
private final String httpFallbackProtocol; | ||
|
||
/* | ||
* Amount of time to wait for acquiring a channel before timing | ||
|
@@ -134,12 +140,14 @@ public class HttpClient { | |
* @param handshakeTimeoutMs if not zero, timeout to use for SSL handshake | ||
* @param name A name to use in logging messages for this client. | ||
* @param logger A logger to use for logging messages. | ||
* @param httpProtocols A list of preferred http protocols (H2 and Http1.1) | ||
*/ | ||
public static HttpClient createMinimalClient(String host, | ||
int port, | ||
SslContext sslCtx, | ||
int handshakeTimeoutMs, | ||
String name, | ||
List<String> httpProtocols, | ||
Logger logger) { | ||
return new HttpClient(host, | ||
port, | ||
|
@@ -149,7 +157,7 @@ public static HttpClient createMinimalClient(String host, | |
true, /* minimal client */ | ||
DEFAULT_MAX_CONTENT_LENGTH, | ||
DEFAULT_MAX_CHUNK_SIZE, | ||
sslCtx, handshakeTimeoutMs, name, logger); | ||
sslCtx, handshakeTimeoutMs, name, httpProtocols, logger); | ||
} | ||
|
||
/** | ||
|
@@ -178,6 +186,7 @@ public static HttpClient createMinimalClient(String host, | |
* @param handshakeTimeoutMs if not zero, timeout to use for SSL handshake | ||
* @param name A name to use in logging messages for this client. | ||
* @param logger A logger to use for logging messages. | ||
* @param httpProtocols A list of preferred http protocols (H2 and Http1.1) | ||
*/ | ||
public HttpClient(String host, | ||
int port, | ||
|
@@ -189,11 +198,12 @@ public HttpClient(String host, | |
SslContext sslCtx, | ||
int handshakeTimeoutMs, | ||
String name, | ||
List<String> httpProtocols, | ||
Logger logger) { | ||
|
||
this(host, port, numThreads, connectionPoolMinSize, | ||
inactivityPeriodSeconds, false /* not minimal */, | ||
maxContentLength, maxChunkSize, sslCtx, handshakeTimeoutMs, name, logger); | ||
maxContentLength, maxChunkSize, sslCtx, handshakeTimeoutMs, name, httpProtocols, logger); | ||
} | ||
|
||
/* | ||
|
@@ -210,6 +220,7 @@ private HttpClient(String host, | |
SslContext sslCtx, | ||
int handshakeTimeoutMs, | ||
String name, | ||
List<String> httpProtocols, | ||
Logger logger) { | ||
|
||
this.logger = logger; | ||
|
@@ -218,6 +229,15 @@ private HttpClient(String host, | |
this.port = port; | ||
this.name = name; | ||
|
||
this.httpProtocols = httpProtocols.size() > 0 ? | ||
httpProtocols : | ||
new ArrayList<>(Arrays.asList(ApplicationProtocolNames.HTTP_2, ApplicationProtocolNames.HTTP_1_1)); | ||
|
||
// If Http1.1 is in the httpProtocols list, we prefer use it as the fallback | ||
// Else we use the last protocol in the httpProtocols list. | ||
this.httpFallbackProtocol = this.httpProtocols.contains(ApplicationProtocolNames.HTTP_1_1) ? | ||
ApplicationProtocolNames.HTTP_1_1 : this.httpProtocols.get(this.httpProtocols.size() - 1); | ||
|
||
this.maxContentLength = (maxContentLength == 0 ? | ||
DEFAULT_MAX_CONTENT_LENGTH : maxContentLength); | ||
this.maxChunkSize = (maxChunkSize == 0 ? | ||
|
@@ -292,6 +312,14 @@ String getName() { | |
return name; | ||
} | ||
|
||
List<String> getHttpProtocols() { | ||
return httpProtocols; | ||
} | ||
|
||
public String getHttpFallbackProtocol() { | ||
return httpFallbackProtocol; | ||
} | ||
|
||
Logger getLogger() { | ||
return logger; | ||
} | ||
|
@@ -354,6 +382,15 @@ public int getFreeChannelCount() { | |
return pool.getFreeChannels(); | ||
} | ||
|
||
/** | ||
* Check if "h2" is in the protocols list | ||
* | ||
* @return true if "h2" is in the protocols list | ||
*/ | ||
public boolean useHttp2() { | ||
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. again.. shouldn't the list be ordered by preference? |
||
return this.httpProtocols.contains(ApplicationProtocolNames.HTTP_2); | ||
} | ||
|
||
/* available for testing */ | ||
ConnectionPool getConnectionPool() { | ||
return pool; | ||
|
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.
While we may need to use ApplicationProtocolNames when talking to Netty there should be no netty class or constant that is part of our own public API. We need to provide our own constants to represent http/2 and 1.1 and map them internally (probably not in this class) to netty when needed