•  

Comment Results

Review Name Created Custom Fields Content
CR-NAS-1 17 Aug 2010

Is this class abstract to disallow class instantiations? Is this the accepted way to do this.
Until now, we have added a private class constructor to enforce this.

CR-NAS-1 12 Aug 2010

Denne fil kom med ved en fejl. Den er egentligt en del af NARC-11 Stop using the JMS queues for queuing snapshot harvests.

CR-NAS-1 12 Aug 2010

Denne fil kom med ved en fejl. Den er egentligt en del af NARC-11 Stop using the JMS queues for queuing snapshot harvests.

CR-NAS-1 12 Aug 2010

Denne fil kom med ved en fejl. Den er egentligt en del af NARC-11 Stop using the JMS queues for queuing snapshot harvests.

CR-NAS-1 12 Aug 2010

Der er ingen relevante tests i denne klasse

CR-NAS-1 17 Aug 2010 Classification: Improvement desirable
Ranking: Minor

This value should not hardcoded, but added as a new setting in HarvesterSettings

CR-NAS-1 12 Aug 2010

Is this really necessary to test whether a set of constant are different and equal to themsefs?

CR-NAS-1 17 Aug 2010 Classification: Missing
Ranking: Minor

Missing @return statement in javadoc

CR-NAS-1 17 Aug 2010

Doesn't this make the line more than 80 characters long?

CR-NAS-1 17 Aug 2010

Doesn't this make the line more than 80 characters long?

CR-NAS-1 17 Aug 2010 Classification: Not conforming to standards
Ranking: Minor

Missing validation of 'queueID' argument

CR-NAS-1 17 Aug 2010 Classification: Missing
Ranking: Minor

Incomplete javadoc statements

CR-NAS-1 17 Aug 2010 Classification: Not conforming to standards
Ranking: Minor

line seems longer than 80 characters

CR-NAS-1 17 Aug 2010

Switching to private constructor

CR-NAS-1 17 Aug 2010 Classification: Not conforming to standards
Ranking: Minor

Line seems longer than 80 characters

CR-BITMAG-3 07 Feb 2011

This class is not thead safe!

CR-BITMAG-3 07 Feb 2011

This is not a general properties map, but a map of the different connection settings. The naming should reflect this.

CR-BITMAG-3 07 Feb 2011

remove outcommented code.

CR-BITMAG-3 07 Feb 2011

The DeliveryMode should be a local constant. Or perhaps a setting?

CR-BITMAG-2 25 Jan 2011

The generated classes are not under source control, as they are build based on the the xsd source. Stuff which can be generated should as a rule not be put under source control.

I've tried to document the message class generation functionality here: Wiki -> Development -> Building the Bitrepository -> Building.

CR-BITMAG-2 25 Jan 2011

looks good!!!
However I cannot see the JAXB generated classes here, but the xsd is ok.

CR-BITMAG-3 07 Feb 2011

Correct the ID. Set to auto-update

CR-BITMAG-3 07 Feb 2011

DeliveryMode should also here be a constant.
Though, why are this set to PERSISTENT, when it elsewhere is set to NON_PERSISTENT? This means, that it is initially set to PERSISTENT in this function, which creates the producer, but the method which retrieves this producer through this function sets it to NON_PERSISTENT. Setting it to PERSISTENT is clearly overridden by the NON_PERSISTENT afterwards.

CR-BITMAG-3 07 Feb 2011

Apparently there is a 'tab' problem in this file. Please replace all tabs with spaces.

CR-BITMAG-3 07 Feb 2011

Change the 'JMSType' according to the type of message

CR-BITMAG-3 07 Feb 2011

The default header is missing!

CR-BITMAG-3 07 Feb 2011

Again with the tabs and spaces.

CR-BITMAG-3 07 Feb 2011

Bad header

CR-BITMAG-3 07 Feb 2011

Again, bad combination of tabs and spaces

CR-BITMAG-3 07 Feb 2011

Missing header

CR-BITMAG-3 07 Feb 2011

Interface description is needed.

CR-BITMAG-3 07 Feb 2011

Bad header

CR-BITMAG-3 07 Feb 2011

Missing class description.

CR-BITMAG-3 07 Feb 2011

Again bad header.

CR-BITMAG-3 07 Feb 2011

Instead of Log4j libraries, we should be using the LogBack libraries for logging through the SLF4J interface.

CR-BITMAG-3 07 Feb 2011

As a general rule, we should run the

mvn license:update-project-license

before committing, which will update all file headers with the correct information.

Perhaps we should consider introducing a precommit script (could be manually run), which is run prior to commiting. This script could also include a run of the regression test.

CR-BITMAG-3 07 Feb 2011

As we discussed in BITMAG-30 Establish Logging, I think we should use the SLF4J, instead of commons logging/log4j. The Dependencies for this are already part of the parent pom. See MessageFactory class for example of usage.

If we agree on this we could also close BITMAG-30 Establish Logging

CR-BITMAG-3 07 Feb 2011

I think the test would be more readable if the test sequence code was explicit, instead of hidden in a TestSingleMessage construction. The test code should reflect the business process being tested, which again is what is stated in the test description. You could use the addStep(String, String) method to make this really verbose.

CR-BITMAG-3 07 Feb 2011

Not commons.logging, switch to SLF4J

CR-BITMAG-3 07 Feb 2011

Just add a throws clause to the method and the test framework will handle this

CR-BITMAG-3 07 Feb 2011

Againg, delegate handling of unexpected messages to the test framework

CR-BITMAG-3 07 Feb 2011

There is a lot of ToDo's in the code. We should think of how to handle these to avoid generating a huge backlog (and somewhat hidden) of outstanding things we need to do.

CR-BITMAG-3 07 Feb 2011

Remove these dependencies, as they are included in the parent pom (though Log4J has been replaced by LogBack here)

CR-BITMAG-3 07 Feb 2011 Classification: Extra (superfluous)
Ranking: Minor

Let's try to avoid empty lines with no reason

CR-BITMAG-3 07 Feb 2011

It looks like this class models the messagebus connection settinngs in general. The name and documentation should reflect this

CR-BITMAG-3 07 Feb 2011 Classification: Improvement desirable
Ranking: Minor

I would much prefer if ConnectionFactory is JMS-agnostic - we may easily bind our implementation tightly to JMS

CR-BITMAG-3 07 Feb 2011

This method retrives the configuration for a messagebus located at a specific url. The method and naming should replect this

CR-BITMAG-3 07 Feb 2011

It seems this class is supposed to be used for a round-robin selection of connections from a number of potential connections. It is the responsibility of the caller to detect a dead connection and request a new one.
This should at the very least be documented in the class documentation.
However, it seems to me that it would be a better idea to make it the responsibility of this class to find a working connection.
However, I think that fallbacks etc. should perhaps be handled outside java, since the message bus software should possibly be capable of replicating instances and providing fallbacks in a manner specific for that type of connection

CR-BITMAG-3 07 Feb 2011

A statically initialised iterator? Seems like a bad idea. Should this be a class variable at all?

CR-BITMAG-3 07 Feb 2011

Actually just the collection of connection configurations