Support of headers to 'HttpJsonRequest'. Adding 'Connection: close' header while pinging workspace agent #7898
Conversation
|
Can one of the admins verify this patch? |
|
ci-test |
|
Can one of the admins verify this patch? |
| @@ -118,6 +121,16 @@ public HttpJsonRequest addQueryParam(@NotNull String name, @NotNull Object value | |||
| return this; | |||
| } | |||
|
|
|||
| public HttpJsonRequest addRequestHeader(@NotNull String name, @NotNull String value) { | |||
There was a problem hiding this comment.
Consider renaming to addHeader. We can not add response header to a request.
| requireNonNull(name, "Required non-null header name"); | ||
| requireNonNull(value, "Required non-null header value"); | ||
| if (headers == null) { | ||
| headers = new ArrayList<>(DEFAULT_HEADERS_LIST_SIZE); |
There was a problem hiding this comment.
Can you elaborate on why you have decided to have this 5 as a default value instead of letting default constructor do his job?
There was a problem hiding this comment.
I have decided not to break existing design of the class (see impl. of query params), I think we can safely remove this param for both lists. WDYT?
There was a problem hiding this comment.
as I understand it was mainly done in order to reduce the initial capacity and avoid recreating array in most cases. Maybe also typical number of query params used in che = 5 ? Anyway, probably the performance impact seems to be insufficient and we might just opt for using default capacity for both query params / headers?
There was a problem hiding this comment.
Yeah, it was definitely an optimization. And author might add a comment to that value to clarify that.
I opt for leaving it with default settings.
There was a problem hiding this comment.
I mean header related one. Let's leave query params optimisation as is
There was a problem hiding this comment.
sounds good - PR has been updated
| @@ -202,6 +216,15 @@ protected DefaultHttpJsonResponse doRequest( | |||
| final HttpURLConnection conn = (HttpURLConnection) new URL(url).openConnection(); | |||
| conn.setConnectTimeout(timeout > 0 ? timeout : 60000); | |||
| conn.setReadTimeout(timeout > 0 ? timeout : 60000); | |||
|
|
|||
| final boolean hasHeaders = headers != null && !headers.isEmpty(); | |||
There was a problem hiding this comment.
I have decided not to break existing design of the class (see processing of query params above). I can rework both if needed ?
There was a problem hiding this comment.
you are free to use the style of code you want as long as it doesn't make the code inefficient or too complex (probably empirical for maintainers values ;) ).
So my comment about this line is just matter of my taste )
Feel free to do as you want here.
And since I approved the PR in the initial state I was OK to merge it at that stage, my comments were just insignificant details.
There was a problem hiding this comment.
can we initialise headers = new ArrayList<>() in constructor to not to have if (headers == null) { all other the code.
There was a problem hiding this comment.
@skabashnyuk I'm quite sure that the initial designer of the class who seems to be really keen on performance and even used custom capacity[1] for query list initialization would like this idea. Basically, the rule of thumb for such cases is either do a massive refactoring or follow the design that is already in place e.g [2] (even if you think it is not smth. you would normally do if implementing from scratch)
[1] https://github.com/eclipse/che/pull/7898/files#diff-570b40ab3f5e69b575585ffb160ef3b5L115
[2] https://github.com/eclipse/che/pull/7898/files#diff-570b40ab3f5e69b575585ffb160ef3b5R201
ibuziuk
force-pushed
the
rh-che-479
branch
2 times, most recently
from
be23079
to
315652c
Compare
…eader while pinging workspace agent Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
|
ci-build |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4395/ |
|
ci-test |
|
ci-test build report: |
|
@ibuziuk can you make che6 pr aswell? |
|
@skabashnyuk sure I will ;-) |
benoitf
added
the
kind/bug
|
PR for che6 #7949 |
What does this PR do?
Adding support of headers to 'HttpJsonRequest'
Adding 'Connection: close' header while pinging workspace agent
Details in redhat-developer/rh-che#479 (comment)
Image used for verification on osio -
ibuziuk/che-server:connection-closeWhat issues does this PR fix or reference?
redhat-developer/rh-che#479
Adding support of headers to 'HttpJsonRequest'
Release Notes
Adding support of headers to 'HttpJsonRequest'