On 3/24/2014 2:10 PM, Christina Fu wrote: Some comments: 1. New codes should be written using Java collection framework (i.e. ArrayList) instead of Vector because it's lightweight, concise, more efficient, and easier to use with other collections. Vector is a legacy class and synchronized by default, but in most cases synchronization is not necessary or already handled by the class using the Vector. And if necessary, ArrayList can be synchronized as well so there's no need to use Vector unless for backward compatibility. Please see the following pages: http://javapostsforlearning.blogspot.com/2013/02/difference-between-array... http://stackoverflow.com/questions/1386275/why-is-java-vector-class-consi... 2. In HttpConnFactory.createConnection() it's not necessary to check whether retConn is null because the CMSEngine.getHttpConnection() will create a new instance which is never null. If it can't create a new instance it will throw an exception. 3. In HttpConnFactory.getConnForOp() there seems to be a possibility the mNumConns and mConns would no longer synced up. // the mNumConns is decreased here mNumConns--; for (int i = 0; i <= mNumConns; i++) { if (...) { mConns.removeElementAt(i); } else { // but here the mConns is not changed } } I'd suggest removing mNumConns and using mConns.size() instead. 4. In getConnForOp() if op is null it will remove an element but will still return a null: private IHttpConnection getConnForOp(String op) { IHttpConnection retConn = null; if (op == null) { retConn = mConns.elementAt(mNumConns); mConns.removeElementAt(mNumConns); } else { ... } return retConn; // return null? } Is this the correct behavior? Or should it return the removed element? To reduce possible similar errors I'd suggest in general to return from the method as soon as early as possible: private IHttpConnection getConnForOp(String op) { if (op == null) { retConn = mConns.elementAt(mNumConns); mConns.removeElementAt(mNumConns); return null; // return immediately } // otherwise continue with the method IHttpConnection retConn = null; ... } 5. In isConnForOp() and getConnForOp() the same element is being looked up multiple times: for (int i = 0; i <= mNumConns; i++) { if ((mConns.elementAt(i).getOp() != null) && op.equals(mConns.elementAt(i).getOp())) { ... } else { if (mConns.elementAt(i).getOp() != null) { ... It would be more efficient and easier to read to do the lookup once then reuse it: for (int i = 0; i <= mNumConns; i++) { String connOp = mConns.get(i).getOp(); if (connOp != null && op.equals(connOp)) { ... } else { if (connOp != null) { ... It's also possible to use for-each statement: for (IHttpConnection conn : mConns) { String connOp = conn.getOp(); if (connOp != null && op.equals(connOp)) { ... } else { if (connOp != null) { ... 6. The boolean literal comparison is not necessary: isConnForOp(op) == false It can be simplified as follows: !isConnForOp(op) The not (!) operator can only be used with boolean. It cannot be used to check null pointer, so there shouldn't be any ambiguity. 7. The following line: mConns.add(mNumConns++, boundconn); can be replaced with: mConns.add(boundconn); because List.add() will always add the element to the end of the list. 8. In HttpConnection the m-prefix and the null initialization is not necessary for the new attribute. Generally it's expected the attribute will have the same name as the setter/getter methods, and all attributes are null by default. protected String op; // null by default public String getOp() { return op; } public HttpConnection(..., String op) { this.op = op; } -- Endi S. Dewata

First of all, thanks to Jack (off-line) and Endi for the review. Attached is a replacement patch that supersedes the original patch. One main thing with this patch is that I took jack's suggestion and looked into possibility of adding uri after the HTTP Connection is created. This is different from the original ca->kra case where the uri is set at the time when HttpConneciton was created (originally, I was trying to preserve that). After looking carefully into it, I found it possible. Therefore, now we keep the connection unattached to any uri, op is no longer needed during the time when HttpConnection was created or retrieved. Before send, however, the uri will need to be set, and that's when op is used to retrieve the uri. I find this approach a lot cleaner. Although due to the change of the approach, many of Endi's comments do not immediately apply to this new patch, I was able to apply indirectly some of the concepts into the code in this patch. Please review. thanks, Christina On 03/24/2014 02:32 PM, Endi Sukma Dewata wrote:

Hi, Sorry for the delayed response. I would have thought that we had something like that on the wiki, and all the information is there but its a little jumbled and confused. I'm going to put together a page that tells you how to go from a vanilla f20 machine to a working Dogtag 10 build. I'll announce it shortly. But in brief, the steps would be: 1. Get source code using git 2. Get dependencies using yum .. I will likely use the build requirements pki-core spec file in pki/specs as an initial guide. 3. Run pki/scripts/compose_pki_core_packages rpms Ade On Tue, 2014-03-25 at 16:31 +0000, Anamitra Dutta Majumdar (anmajumd) wrote:

Hi Ade, Thanks a lot for putting up this wiki.. -Anamitra On 3/28/14 9:22 AM, "Ade Lee" <alee(a)redhat.com&gt; wrote: