Alexander Wels has posted comments on this change. Change subject: services: add proxy servlet ......................................................................
Patch Set 4: (5 comments) Any unit tests for this? http://gerrit.ovirt.org/#/c/30740/4/backend/manager/modules/services/src/main/java/org/ovirt/engine/core/services/ProxyServlet.java File backend/manager/modules/services/src/main/java/org/ovirt/engine/core/services/ProxyServlet.java: Line 49: return r; Line 50: } Line 51: Line 52: @Override Line 53: public void init() throws ServletException { Probably want to call: super.init(config); Line 54: setVerifyHost(getConfigBoolean(VERIFY_HOST_PRM)); Line 55: setVerifyChain(getConfigBoolean(VERIFY_CHAIN_PRM)); Line 56: setHttpsProtocol(getConfigString(HTTPS_PROTOCOL_PRM)); Line 57: setTrustManagerAlgorithm(getConfigString(TRUST_MANAGER_ALGORITHM_PRM)); http://gerrit.ovirt.org/#/c/30740/4/backend/manager/modules/uutils/pom.xml File backend/manager/modules/uutils/pom.xml: Line 31: </dependency> Line 32: Line 33: <dependency> Line 34: <groupId>org.jboss.spec.javax.servlet</groupId> Line 35: <artifactId>jboss-servlet-api_3.0_spec</artifactId> This is provided by jboss, no need to include it. <scope>provided</scope> Line 36: </dependency> Line 37: Line 38: </dependencies> Line 39: http://gerrit.ovirt.org/#/c/30740/4/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 37: private Integer readTimeout; Line 38: private String url; Line 39: Line 40: protected static long copy(final InputStream input, final OutputStream output) throws IOException { Line 41: final byte[] buffer = new byte[8024]; Should that be 8192? 8024 seems like a strange number. Line 42: long count = 0; Line 43: int n; Line 44: while ((n = input.read(buffer)) != -1) { Line 45: output.write(buffer, 0, n); Line 128: } else { Line 129: targetURL += "&"; Line 130: } Line 131: targetURL += request.getQueryString(); Line 132: } Can we extract this into a method, something like mergeQuery? String targetURL = mergeQuery(url, request.getQueryString()); private String mergeQuery(String baseUrl, String query) { String targetUrl = baseUrl; if (query != null) { URL u = new URL(baseUrl); if (u.getQuery() == null) { targetURL += "?"; } else { targetURL += "&"; } targetURL += query; } return targetURL; } Line 133: URLConnection connection = new URL(targetURL).openConnection(); Line 134: connection.setDoInput(true); Line 135: connection.setDoOutput(false); Line 136: connection.setAllowUserInteraction(false); Line 183: } Line 184: } catch(Exception e) { Line 185: throw new ServletException(e); Line 186: } Line 187: } You probably want to extract all of the above into a 'createConnection' method. final URLConnection connection = createConnection(); connection.connect(); private URLConnection createConnection() { URLConnection connection; ... all the stuff above ... return connection; } Line 188: connection.connect(); Line 189: try { Line 190: if (connection instanceof HttpURLConnection) { Line 191: response.setStatus(((HttpURLConnection)connection).getResponseCode()); -- To view, visit http://gerrit.ovirt.org/30740 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia26768ec7a0e06a20f657cae40f6a51303970b44 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: Yaniv Dary <[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
