Yair Zaslavsky has posted comments on this change. Change subject: uutils: Extract connection class ......................................................................
Patch Set 2: (8 comments) http://gerrit.ovirt.org/#/c/33479/2/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/http/Connection.java File backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/http/Connection.java: Line 90: return this; Line 91: } Line 92: Line 93: Line 94: public void create() throws IOException, GeneralSecurityException { > won't it better to return URLConnection from create and rename to createCon As this is in a package of http, I would pefer HttpURLConnection. I would like to avoid callers do the "instanceof" check, when they know they want to use an Http connection. In addition maybe this should actually return Connection, to allow method chaining with setXXX. Regarding the name, well, this is a method of Connection, I assume create creates a connection, no ? Line 95: connection = new URL(url).openConnection(); Line 96: connection.setDoInput(true); Line 97: connection.setDoOutput(false); Line 98: connection.setAllowUserInteraction(false); Line 151: return connection; Line 152: } Line 153: Line 154: public int getResponseCode() throws IOException { Line 155: return asHttpUrlConnection().getResponseCode(); > why can't the caller use this one liner? Done Line 156: } Line 157: Line 158: public Map<String, List<String>> getHeaderFields() { Line 159: return asHttpUrlConnection().getHeaderFields(); Line 156: } Line 157: Line 158: public Map<String, List<String>> getHeaderFields() { Line 159: return asHttpUrlConnection().getHeaderFields(); Line 160: } > why can't the caller use this one liner? Done Line 161: Line 162: public void connect() throws IOException { Line 163: asHttpUrlConnection().connect(); Line 164: } Line 159: return asHttpUrlConnection().getHeaderFields(); Line 160: } Line 161: Line 162: public void connect() throws IOException { Line 163: asHttpUrlConnection().connect(); > why can't the caller use this one liner? Done Line 164: } Line 165: Line 166: public InputStream getInputStream() throws IOException { Line 167: return asHttpUrlConnection().getInputStream(); Line 163: asHttpUrlConnection().connect(); Line 164: } Line 165: Line 166: public InputStream getInputStream() throws IOException { Line 167: return asHttpUrlConnection().getInputStream(); > why can't the caller use this one liner? Done Line 168: } Line 169: Line 170: public void disconnect() { Line 171: asHttpUrlConnection().disconnect(); Line 171: asHttpUrlConnection().disconnect(); Line 172: } Line 173: Line 174: Line 175: protected static long copy(final InputStream input, final OutputStream output) throws IOException { > not sure this is required here... correct, i will remove. Line 176: final byte[] buffer = new byte[8 * 1024]; Line 177: long count = 0; Line 178: int n; Line 179: while ((n = input.read(buffer)) != -1) { http://gerrit.ovirt.org/#/c/33479/2/backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/servlet/ProxyServletBase.java File backend/manager/modules/uutils/src/main/java/org/ovirt/engine/core/uutils/servlet/ProxyServletBase.java: Line 88: .setTrustStorePassword(trustStorePassword) Line 89: .setTrustStoreType(trustStoreType) Line 90: .setURL(url.toString()) Line 91: .setVerifyChain(verifyChain) Line 92: .setVerifyHost(verifyHost); > where do you use create? Done Line 93: } Line 94: Line 95: private String mergeQuery(String url, String queryString) throws MalformedURLException { Line 96: String ret = url; Line 136: Line 137: if (url == null) { Line 138: response.sendError(response.SC_NOT_FOUND, "Cannot proxy, no URL is configured."); Line 139: } else { Line 140: Connection connection; > why do we need Connection here and we cannot hold URLConnection? We should hold HttpURLConnection , IMHO Line 141: try { Line 142: connection = createConnection(new URL(mergeQuery(url, request.getQueryString()))); Line 143: } catch (Exception e) { Line 144: throw new ServletException(e); -- To view, visit http://gerrit.ovirt.org/33479 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I85ea4e7301b3a018b0438fff25cefad80ebd7256 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
