= Review (NS-65): Bug 555 and other related bugs =
|| Author || Søren ||
|| Moderator || Søren ||
|| State || Closed ||
== Objectives ==
{{{
Review the changes introduced by KFC to JMSConnection, and related classes.
Note that a problem has already been identifed in the JMSConnection code. One of our unittests provokes this NPE:
Exception in thread "Thread-285" java.lang.NullPointerException
        at dk.netarkivet.common.distribute.JMSConnection.getProducer(JMSConnection.java:445)
        at dk.netarkivet.common.distribute.JMSConnection.doSend(JMSConnection.java:550)
        at dk.netarkivet.common.distribute.JMSConnection.sendMessage(JMSConnection.java:367)
        at dk.netarkivet.common.distribute.JMSConnection.send(JMSConnection.java:200)
        at dk.netarkivet.archive.bitarchive.distribute.BitarchiveMonitorServer.doBatchReply(BitarchiveMonitorServer.java:252)
        at dk.netarkivet.archive.bitarchive.distribute.BitarchiveMonitorServer.access$1(BitarchiveMonitorServer.java:232)
        at dk.netarkivet.archive.bitarchive.distribute.BitarchiveMonitorServer$2.run(BitarchiveMonitorServer.java:218)
}}}
== Summary ==
{{{
Have now reviewed this code. Probably needs to be reviewed again when the author returns from vacation.
Have found problems with the code, that needs to be patched.
Followup by SVC
}}}
'''General comments''':
|| '''Description''' || '''Classification''' || '''Status''' ||


=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/ArcRepositoryServer.java', revision 889 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/JMSArcRepositoryClient.java', revision 889 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
=== Comments on file 'trunk/src/dk/netarkivet/common/distribute/JMSConnection.java', revision 890 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Exception in thread "Thread-285" java.lang.NullPointerException   at dk.netarkivet.common.distribute.JMSConnection.getProducer(JMSConnection.java:445)    at dk.netarkivet.common.distribute.JMSConnection.doSend(JMSConnection.java:550)         at dk.netarkivet.common.distribute.JMSConnection.sendMessage(JMSConnection.java:367)    at dk.netarkivet.common.distribute.JMSConnection.send(JMSConnection.java:200)   at dk.netarkivet.archive.bitarchive.distribute.BitarchiveMonitorServer.doBatchReply(BitarchiveMonitorServer.java:252)   at dk.netarkivet.archive.bitarchive.distribute.BitarchiveMonitorServer.access$1(BitarchiveMonitorServer.java:232)       at dk.netarkivet.archive.bitarchive.distribute.BitarchiveMonitorServer$2.run(BitarchiveMonitorServer.java:218) || Cosmetic || OK ||
|| 65 || consumer key (two words rigtht?) || Cosmetic || OK ||
|| 71 || JMX_MAX_TRIES || Cosmetic || REJECTED ||
|| 199 || log -> + ". To:" + msg.getTo() || Cosmetic || OK ||
|| 213 || log -> " To : " + to || Cosmetic || OK ||
|| 261 || Missing javadoc for method removeListener() || Cosmetic || OK ||
|| 269 || Add todo to the javadoc, that this method sets the producers and consumers datastructures to null without initializing them again elsewhere This is problematic when cleanup() is called as part of the reconnection procedure || Cosmetic || OK ||
|| 442 || This method currently throws an NPE in one of our unittests. See general comment above for details || Cosmetic || OK ||
|| 478 || Add missing javadoc to explain this helper method || Cosmetic || OK ||
|| 487 || Add missing javadoc to explain this helper method || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/ArcRepository.java', revision 889 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
=== Comments on file 'trunk/src/dk/netarkivet/common/utils/JMXUtils.java', revision 889 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 255-256 || Move "+"' at end of line to next line || Cosmetic || OK ||
|| 374-375 || Move || at end of line to next line || Cosmetic || OK ||
|| 387, 389 || Too long line || Cosmetic || OK ||