Added RobustHTTPClient as a beta API #9

jglick · 2018-06-05T21:03:57Z

Subsumes #8.

@carlossg @kuisathaverat @oleg-nenashev @dwnusbaum

oleg-nenashev · 2018-06-06T09:15:19Z

src/main/java/io/jenkins/plugins/httpclient/RobustHTTPClient.java

+ * THE SOFTWARE.
+ */
+
+package io.jenkins.plugins.httpclient;


Package name does differ from artifactId. I would propose apache.httpcomponents.client.api or so.
OTOH the code does not use the library on its own. So more reviews may be required

That package name is pretty awkward, and also gives the false impression that this is Apache code.

Using the artifactId ~ shortName, in some modified form, as a final package component is indeed typical. This particular artifactId is quite unwieldy though. Also I am not sure there is a solid convention for “miscellaneous source packages added to a library wrapper”, which is not really the same as “main source package of a regular plugin”.

I assume Oleg meant io.jenkins.plugins.apache.httpcomponents.client.api

Normally - is converted to _ or camel casing when constructing a package component from an artifactId, so something like io.jenkins.plugins.apache_httpcomponents_client_4_api. Pretty ugly.

kuisathaverat · 2018-06-06T09:21:29Z

src/main/java/io/jenkins/plugins/httpclient/RobustHTTPClient.java

+    public void connect(String whatConcise, String whatVerbose, ConnectionCreator connectionCreator, ConnectionUser connectionUser, TaskListener listener) throws IOException, InterruptedException {
+        AtomicInteger responseCode = new AtomicInteger();
+        int attempt = 1;
+        while (true) {


a little complex to follow, there are too many alternatives in the same method

FWIW the original version, which depended on the guava-retrying library that is supposed to wrap this logic in a pretty higher-level API, was just as long and (I think) harder to follow.

kuisathaverat · 2018-06-06T09:25:27Z

src/main/java/io/jenkins/plugins/httpclient/RobustHTTPClient.java

+                try {
+                    executors.submit(() -> {
+                        responseCode.set(0);
+                        try (CloseableHttpClient client = HttpClients.createSystem()) {


Why not join the two try?

They have different scopes.

kuisathaverat · 2018-06-06T09:26:48Z

src/main/java/io/jenkins/plugins/httpclient/RobustHTTPClient.java

+                        try (CloseableHttpClient client = HttpClients.createSystem()) {
+                            try (CloseableHttpResponse response = connectionCreator.connect(client)) {
+                                StatusLine statusLine = response.getStatusLine();
+                                responseCode.set(statusLine != null ? statusLine.getStatusCode() : 0);


maybe make the ? in the declaration is more clear

Sorry, what?

nothing, I read response.getStatusLine() instead statusLine.getStatusCode()

kuisathaverat · 2018-06-06T09:27:55Z

src/main/java/io/jenkins/plugins/httpclient/RobustHTTPClient.java

+                            try (CloseableHttpResponse response = connectionCreator.connect(client)) {
+                                StatusLine statusLine = response.getStatusLine();
+                                responseCode.set(statusLine != null ? statusLine.getStatusCode() : 0);
+                                if (responseCode.get() < 200 || responseCode.get() >= 300) {


what happens if there is a redirection 301/302?

Treated as a failure currently. The existing use cases do not require following redirects. It would be possible to handle I think if there were some use case.

kuisathaverat · 2018-06-06T09:29:19Z

src/main/java/io/jenkins/plugins/httpclient/RobustHTTPClient.java

+                                            Header contentEncoding = entity.getContentEncoding();
+                                            diag = IOUtils.toString(err, contentEncoding != null ? contentEncoding.getValue() : null);
+                                        }
+                                    } else {


maybe you can initialized diag to null and remove the else

Could, I just have a mild preference for effectively final variables.

kuisathaverat · 2018-06-06T09:30:52Z

src/main/java/io/jenkins/plugins/httpclient/RobustHTTPClient.java

+                                    } else {
+                                        diag = null;
+                                    }
+                                    throw new AbortException(String.format("Failed to %s, response: %d %s, body: %s", whatVerbose, responseCode.get(), statusLine != null ? statusLine.getReasonPhrase() : "?", diag));


if you make the ? in the declaration of statusLine you have not repeat the code here

Still not sure I am following. You mean, make statusLine nonnull by creating a fake object as a fallback?

forget I mixed method names

kuisathaverat · 2018-06-06T09:34:05Z

src/main/java/io/jenkins/plugins/httpclient/RobustHTTPClient.java

+                        }
+                        return null; // success
+                    }).get(timeout, TimeUnit.SECONDS);
+                } catch (TimeoutException x) {


you can move it a level up and move the Exception wrapper to a method, this makes the code simpler

Thought about doing that but did not see a way to do so without making things much more complicated.

kuisathaverat · 2018-06-06T09:39:52Z

src/main/java/io/jenkins/plugins/httpclient/RobustHTTPClient.java

+     * @return the same, but with any query string concealed
+     */
+    public static String sanitize(URL url) {
+        return url.toString().replaceFirst("[?].+$", "?…");


not sure, but Could be interesting to remove the stuff between @ and //? I mean, if we have the URL http://U:P@HOST?QUERY we replace it with http://U:P@HOST?..., it is possible to make the same with the user and password and transform the URL to http://..@HOST?...

passwords should be removed, user would be ok

also, should it be three chars ... or one unicode char …? I think the former

URL.userInfo is generally opaque, not necessarily user:pass. It could be masked if needed. Currently there is no use case for doing so, as (semi-)secrets are passed in the query string.

carlossg

IMHO if this is exposed it lacks docs on how to use it and an example
We should move some of the mock tests from downstream to here

carlossg · 2018-06-06T11:14:48Z

src/main/java/io/jenkins/plugins/httpclient/RobustHTTPClient.java

+     * @return the same, but with any query string concealed
+     */
+    public static String sanitize(URL url) {
+        return url.toString().replaceFirst("[?].+$", "?…");


passwords should be removed, user would be ok

also, should it be three chars ... or one unicode char …? I think the former

jglick · 2018-06-06T14:11:32Z

it lacks docs on how to use it

Well, there is Javadoc…

and an example

The upload and download methods are examples of using the base method.

We should move some of the mock tests from downstream to here

Not possible I think.

jglick · 2018-06-07T15:42:30Z

@carlossg do you consider this OK to merge? (I cannot request you as a reviewer.) I think would be up to @oleg-nenashev or @dwnusbaum to actually do so.

carlossg · 2018-06-07T15:52:55Z

src/main/java/io/jenkins/plugins/httpclient/RobustHTTPClient.java

+    public static String sanitize(URL url) {
+        try {
+            URI orig = url.toURI();
+            return new URI(orig.getScheme(), orig.getUserInfo() != null ? "…" : null, orig.getHost(), orig.getPort(), orig.getPath(), orig.getQuery() != null ? "…" : null, orig.getFragment()).toString();


I asked before, is … intended or should be ... ? will unicode behave correctly?

Either should be fine. This is just for use in build log messages, where we already often print ‘’ or » etc. (Cf. jenkinsci/workflow-job-plugin#89.)

dwnusbaum · 2018-06-07T19:49:42Z

src/main/java/io/jenkins/plugins/httpclient/RobustHTTPClient.java

+    }
+
+    /**
+     * Maximum number of seconds between upload/download attempts.


nit: I think it would be good to use the same units or make the user pass a ChronoUnit/TimeUnit for waitMultiplier and waitMaximum (maybe also timeout) to avoid potential confusion.

…he API.

dwnusbaum

Looks good to me. I would like to see some tests for the general logic, but it seems like that would need a mocking framework which might get pretty nasty. I have no strong opinion about the package name.

jglick · 2018-06-08T13:27:08Z

tests for the general logic, but it seems like that would need a mocking frameworks

Not necessarily a mocking framework, just a local server…but NetworkTest in artifact-manager-s3 already exercises all of this stuff. The original code was anyway written in TDD style against that test. (Originally using the guava-retrying library, then throwing that out and reimplemented directly with a loop because exponential backoff is not really that complicated.)

oleg-nenashev

I still do not like the current package name, but I am fine with it if @dwnusbaum is fine.

Added RobustHTTPClient as a beta API.

68d97eb

jglick mentioned this pull request Jun 5, 2018

[JENKINS-50597] Network behavior tuning IV jenkinsci/artifact-manager-s3-plugin#42

Merged

10 tasks

jglick added a commit to jenkinsci/artifact-manager-s3-plugin that referenced this pull request Jun 5, 2018

Picking up jenkinsci/apache-httpcomponents-client-4-api-plugin#9.

b45134a

More methods.

4a9f3bc

jglick added a commit to jglick/jenkins that referenced this pull request Jun 5, 2018

Removing FilePath.copyFromRemotely in favor of jenkinsci/apache-httpc…

1688b78

…omponents-client-4-api-plugin#9.

This was referenced Jun 5, 2018

Using RobustHTTPClient utility for artifact downloads jenkinsci/copyartifact-plugin#104

Merged

[JENKINS-52417] - Removing FilePath.copyFromRemotely jenkinsci/jenkins#3480

Merged

oleg-nenashev requested review from dwnusbaum and oleg-nenashev and removed request for oleg-nenashev June 6, 2018 09:13

oleg-nenashev reviewed Jun 6, 2018

View reviewed changes

kuisathaverat reviewed Jun 6, 2018

View reviewed changes

carlossg reviewed Jun 6, 2018

View reviewed changes

jglick added 2 commits June 6, 2018 11:00

Make sanitize also mask out any userInfo component, just in case.

67dc94b

Fuller Javadoc.

2af69a0

oleg-nenashev self-requested a review June 6, 2018 19:12

kuisathaverat approved these changes Jun 6, 2018

View reviewed changes

Merge branch 'master' into RobustHTTPClient

2ea0b6b

carlossg reviewed Jun 7, 2018

View reviewed changes

dwnusbaum reviewed Jun 7, 2018

View reviewed changes

carlossg approved these changes Jun 7, 2018

View reviewed changes

@dwnusbaum suggests unifying time units and making them explicit in t…

5d1d316

…he API.

carlossg approved these changes Jun 8, 2018

View reviewed changes

jglick mentioned this pull request Jun 8, 2018

Matching change jenkinsci/artifact-manager-s3-plugin#48

Merged

dwnusbaum approved these changes Jun 8, 2018

View reviewed changes

oleg-nenashev approved these changes Jun 8, 2018

View reviewed changes

dwnusbaum merged commit 085500f into jenkinsci:master Jun 8, 2018

jglick deleted the RobustHTTPClient branch June 8, 2018 14:14

Added RobustHTTPClient as a beta API #9

Added RobustHTTPClient as a beta API #9

Uh oh!

Conversation

jglick commented Jun 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuisathaverat Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlossg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick commented Jun 6, 2018

Uh oh!

jglick commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwnusbaum left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick commented Jun 8, 2018

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

kuisathaverat Jun 6, 2018 •

edited

Loading

jglick Jun 6, 2018 •

edited

Loading

dwnusbaum left a comment •

edited

Loading