> On 2011-02-18 07:55:41, Robbie Gemmell wrote: > > 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); > > kevinconner wrote: > The code is based on the hornetq adapter and I have permission to donate > this version to apache qpid (I work for Red Hat, the copyright owner). I > believe we have applied the appropriate header based on your link. > > Apologies for the coding style but I was asked to get a JCA adapter > working and passing the TCK as quickly as possible, no attempt has been made > to integrate it into the build nor reformat according to coding styles. > There are better folk than I that can handle this part. > > Apologies again for the logging, this is an artifact of the original > code. I will tidy this up and correct the definitions.
Ok, yes in that case I believe you have applied the correct header. That you were even working on this or what it was based on were not things I knew of at the time of comment, which is why I was asked (always best to in those situations..) :) - Robbie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/432/#review232 ----------------------------------------------------------- On 2011-02-17 12:17:18, Andrew Stitcher wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/432/ > ----------------------------------------------------------- > > (Updated 2011-02-17 12:17:18) > > > Review request for qpid. > > > Summary > ------- > > 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. > > > This addresses bug 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 > > Diff: https://reviews.apache.org/r/432/diff > > > Testing > ------- > > > Thanks, > > Andrew > >
