Forwarding my Review Board comment, since I was an idiot and didnt use the address I'm subscribed to the dev list with when registering :)
Robbie ---------- Forwarded message ---------- From: "Robbie Gemmell" <[email protected]> To: "Andrew Stitcher" <[email protected]>, "Robbie Gemmell" < [email protected]>, "qpid" <[email protected]> Date: Fri, 18 Feb 2011 15:55:41 -0000 Subject: Re: Review Request: JCA resource adapter for Qpid This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/432/ If the code is based on something written by another author originally (most of the files crediting other authors suggests this is this case?) I think it should be commited in its original state first so that we can verify the base usage is legitimate and also track the changes made to transform it. Also, if the code was developed by others originally, should it have the ASF attributed version of the Apache Licence header? See http://www.apache.org/legal/src-headers.html. The code doesn't confirm to some parts of the general code style used by (most of) the Java codebase (described at https://cwiki.apache.org/qpid/java-coding-standards.html), likely due to its seeming origin but still worth noting. Specifically the instance variable naming doesn't use prefixed underscores to differentiate them from local variable names (helps identifying/preventing variable shadowing). This is The style used for the logging is an example of where changes should be made, eg: if (ConnectionFactoryProperties.trace) { ConnectionFactoryProperties.log.trace("getDefaultUsername()"); } ConnectionFactoryProperties.trace and ConnectionFactoryProperties.log.trace are a lot longer and less clear (initially I thought it was doing something worse and accessing public variables in another class) than just using _trace and _log.trace if following the instance variable style mentioned above. Also, ConnectionFactoryProperties.trace as noted above is defined statically which means it can't be manipulated to enable trace logging while running, although it may be (appreciably?) faster than doing the normally used alternative: if(_log.isTraceEnabled()). This would instead give: if (_log.isTraceEnabled()) { _log.trace("getDefaultUsername()"); } Also, in that particular class (ConnectionFactoryProperties) the Logger is incorrectly defined as using the log hierarchy for QpidRAMCFProperties.class: private static final Logger log = LoggerFactory.getLogger(QpidRAMCFProperties.class); - Robbie On February 17th, 2011, 12:17 p.m., Andrew Stitcher wrote: Review request for qpid. By Andrew Stitcher. *Updated 2011-02-17 12:17:18* Description This is the first review candidate for a qpid JCA resource adapter. So far no build infrastructure is included. I'd also like an opinion as to whether java/jca is the appropriate name for this (I'm thinking perhaps java/ra would be more usual). Any and all comments welcome. *Bugs: * QPID-3044 <https://issues.apache.org/jira/browse/QPID-3044> Diffs - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/ConnectionFactoryObjectFactory.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/ConnectionFactoryProperties.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRABytesMessage.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAConnectionFactory.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAConnectionFactoryImpl.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAConnectionManager.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAConnectionMetaData.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAConnectionRequestInfo.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRACredential.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAException.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRALocalTransaction.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAMCFProperties.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAManagedConnection.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAManagedConnectionFactory.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAMapMessage.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAMessage.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAMessageConsumer.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAMessageListener.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAMessageProducer.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAMetaData.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAObjectMessage.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAProperties.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAQueueReceiver.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAQueueSender.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRASession.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRASessionFactory.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRASessionFactoryImpl.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAStreamMessage.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRATextMessage.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRATopicPublisher.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRATopicSubscriber.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAXAResource.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidResourceAdapter.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/Util.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/inflow/QpidActivation.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/inflow/QpidActivationSpec.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/inflow/QpidMessageHandler.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/tm/JBossTransactionManagerLocator.java (PRE-CREATION) - trunk/qpid/java/jca/src/main/resources/META-INF/ra.xml (PRE-CREATION) - trunk/qpid/java/lib/geronimo-j2ee-connector_1.5_spec-2.0.0.jar (UNKNOWN) - trunk/qpid/java/lib/geronimo-jta_1.1_spec-1.1.1.jar (UNKNOWN) View Diff <https://reviews.apache.org/r/432/diff/>
