8296804: Document HttpClient configuration properties in java.net.http module-info #11241
Conversation
|
👋 Welcome back michaelm! A progress list of the required criteria for merging this PR into |
|
@Michael-Mc-Mahon The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
There was a problem hiding this comment.
I think we should limit ourselves to document properties that are of interest to users of the API - so probably looking at what is documented in the Java Core Libraries Developer's Guide
| * <li><p><b>{@systemProperty jdk.internal.httpclient.debug}</b> (default: false)<br> | ||
| * Enables general debug tracing. | ||
| * </li> | ||
| * <li><p><b>{@systemProperty jdk.internal.httpclient.websocket.debug}</b> (default: false)<br> | ||
| * Enables general websocket debug tracing. | ||
| * </li> | ||
| * <li><p><b>{@systemProperty jdk.internal.httpclient.hpack.debug}</b> (default: false)<br> | ||
| * Enables general HTTP/2 HPACK debug tracing. | ||
| * </li> |
There was a problem hiding this comment.
These three *.debug properties are too low level and should not be documented.
| * <li><p><b>{@systemProperty jdk.httpclient.keepalive.timeout}</b> (default: 1200)<br> | ||
| * The number of seconds to keep idle HTTP/1.1 connections alive in the keep alive cache. | ||
| * </li> |
There was a problem hiding this comment.
This is no longer strictly true. Also jdk.httpclient.keepalive.timeout.h2 should be documented, see JDK-8295649
|
Also it should be made clear that these system properties should be specified on the java command line (no guarantee that a value specified with System.setProperty() willl be taken into account) and are implementation specific properties, not part of the specification. That is - another implementation of |
| * <li><p><b>{@systemProperty jdk.httpclient.maxstreams}</b> (default: 100)<br> | ||
| * The maximum number of HTTP/2 streams per connection. | ||
| * </li> |
There was a problem hiding this comment.
It might be useful to be a bit more precise here: this is the maximum number of concurrent push streams that a server is allowed to initiate on an HTTP/2 connection
There was a problem hiding this comment.
Right, good point. It's the client telling the server how many streams it is allowed to open. So, that implies push streams only
| * <li><p><b>{@systemProperty jdk.internal.httpclient.selectorTimeout}</b> (default: 3000)<br> | ||
| * The selector thread wakes up repeatedly after this number of milliseconds when there is no activity. | ||
| * This allows the thread to terminate when no longer needed. The value provided is limited | ||
| * to the range from 1 to 1200(???) seconds. | ||
| * </li> |
There was a problem hiding this comment.
Do we need to document this property? If yes then what is the meaning of the (???) question marks?
There was a problem hiding this comment.
The question mark was a place holder to query the value range. 1200 seemed extremely high. But, I think we don't need to document it at all.
| * <li><p><b>{@systemProperty jdk.httpclient.sendBufferSize}</b> (default: operating system default)<br> | ||
| * The HTTP client socket send buffer size. Values less than or equal to zero are ignored. | ||
| * </li> |
There was a problem hiding this comment.
I wonder if we should make a note that attempting to specify a receive or send buffer size may be detrimental to performance as it might prevent the underlying system to optimize sending and receiving. Also a link to the corresponding StandardOptions may be in order (same for receive buffer size above).
There was a problem hiding this comment.
I would agree with linking to the StandardSocketOption descriptions, as that would also help to distinguish the send buffer option from the write buffer option, which sounds similar. In my view, any discussion of performance trade-offs should be in the StandardSocketOption apidoc rather than here.
There was a problem hiding this comment.
Sounds reasonable to me. BTW - you seem to have tripped jcheck due to trailing white spaces.
| * <li><p><b>{@systemProperty jdk.internal.httpclient.disableHostnameVerification}</b> (default: false)<br> | ||
| * If true, hostname verification in SSL certificates is disabled. This is a system property only. | ||
| * </li> |
There was a problem hiding this comment.
Should we emphasize that it is intended for testing purposes?
| * <li><p><b>{@systemProperty jdk.http.auth.proxying.disabledSchemes}</b> (default: see conf/net.properties)<br> | ||
| * A comma separated list of HTTP authentication scheme names, that are disallowed for use by | ||
| * the HTTP client implementation, for HTTP proxying. | ||
| * </li> | ||
| * <li><p><b>{@systemProperty jdk.http.auth.tunneling.disabledSchemes}</b> (default: see conf/net.properties)<br> | ||
| * A comma separated list of HTTP authentication scheme names, that are disallowed for use by | ||
| * the HTTP client implementation, for HTTP CONNECT tunneling. | ||
| * </li> |
There was a problem hiding this comment.
Should we provide a link to net-properties.html here?
There was a problem hiding this comment.
I don't see the benefit in that?
There was a problem hiding this comment.
I believe these properties are documented there already.
There was a problem hiding this comment.
Ah - no. They are not. My mistake. They are documented in conf/net.properties but we can't have a link to that.
|
/csr needed |
|
@Michael-Mc-Mahon an approved CSR request is already required for this pull request. |
| * <li>frames</li> | ||
| * <li>ssl</li> | ||
| * <li>trace</li> | ||
| * <li>channel</li> |
There was a problem hiding this comment.
Hello Michael, is it intentional that we haven't documented content in this list?
There was a problem hiding this comment.
Thanks for noticing that omission Jai!
| * </li> | ||
| * <li><p><b>{@systemProperty jdk.httpclient.connectionWindowSize}</b> (default: 2^26)<br> | ||
| * The HTTP/2 client connection window size in bytes. The maximum size is 2^31-1. This value cannot be | ||
| * smaller than the stream window size. |
There was a problem hiding this comment.
Should this additionally point to the jdk.httpclient.windowsize system property? Something like This value cannot be smaller than the stream window size, which can be configured through {@code jdk.httpclient.windowsize} system property?
| * </li> | ||
| * <li><p><b>{@systemProperty jdk.internal.httpclient.disableHostnameVerification}</b> (default: false)<br> | ||
| * If true, hostname verification in SSL certificates is disabled. This is a system property only and not | ||
| * available in {@code conf/net.properties}. It is provided for testing purposes only. |
There was a problem hiding this comment.
From what I can see in the code jdk.internal.net.http.common.Utils#hostnameVerificationDisabledValue(), we consider an empty value for this property to imply true. Perhaps we should note that here?
| * Enables high-level logging of various events through the Java Logging API (which is contained in the | ||
| * package java.util.logging). The value contains a comma-separated list of any of the following items: |
There was a problem hiding this comment.
java.util.logging is actually only the default backend, so I'd suggest to reword as:
* Enables high-level logging of various events through the {@linkplain java.lang.System.Logger Platform Logging API}.
* The value contains ...
| * <li>all</li> | ||
| * </ul><br> | ||
| * Specifying an item adds it to the HTTP client's log. For example, if you specify the following value, | ||
| * then the Java Logging API logs all possible HTTP Client events:<br> |
There was a problem hiding this comment.
Same here - use Platform Logging API rather than Java Logging API.
| * <li><p><b>{@systemProperty jdk.httpclient.keepalive.timeout}</b> (default: 30)<br> | ||
| * The number of seconds to keep idle HTTP connections alive in the keep alive cache. This property | ||
| * applies to both HTTP/1.1 and HTTP/2. The value for HTTP/2 can be overridden with the | ||
| * jdk.httpclient.keepalive.timeout.h2 property. |
There was a problem hiding this comment.
{@code jdk.httpclient.keepalive.timeout.h2} ?
| * </li> | ||
| * <li><p><b>{@systemProperty jdk.httpclient.keepalive.timeout.h2}</b> (default: see below)<br> | ||
| * The number of seconds to keep idle HTTP/2 connections alive. If not set, then the | ||
| * jdk.httpclient.keepalive.timeout setting is used. |
There was a problem hiding this comment.
{@code jdk.httpclient.keepalive.timeout} ?
|
Will push an update including the comments above and then a further update with just reformatting changes. |
|
CSR updated now. |
| @@ -72,12 +73,13 @@ | |||
| * The HTTP/2 client maximum HPACK header table size in bytes. | |||
| * </li> | |||
| * <li><p><b>{@systemProperty jdk.httpclient.HttpClient.log}</b> (default: none)<br> | |||
| * Enables high-level logging of various events through the Java Logging API (which is contained in the | |||
| * package java.util.logging). The value contains a comma-separated list of any of the following items: | |||
| * Enables high-level logging of various events through the {@link java.lang.System.Logger Platform Logging | |||
There was a problem hiding this comment.
Should be {@linkplain } otherwise the text will be in fixed font.
|
@Michael-Mc-Mahon This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 26 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
|
Going to push as commit 48017b1.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@Michael-Mc-Mahon Pushed as commit 48017b1. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi,
Could I get the following doc change reviewed please? It documents the system/networking properties used by the java.net.http HTTP client in its module-info. (CSR to follow)
Thanks,
Michael
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11241/head:pull/11241$ git checkout pull/11241Update a local copy of the PR:
$ git checkout pull/11241$ git pull https://git.openjdk.org/jdk pull/11241/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11241View PR using the GUI difftool:
$ git pr show -t 11241Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11241.diff