Review (NS-129): Changes caused by progress in Assignment B.2.2b.

Author	Jonas
Moderator	Jonas
State	Closed

Objectives

The DatabaseBasedActiveBitPreservation is not finished, and it is therefore not important the it works completely.
It has to be ensured that current functionality still works.

Total Time Used (Coding,Documentation,Review):

Time use (Coding,Documentation,Review)
JOLF: 3 md
SVC: 0.5 md

General comments:

Description	Classification	Status
Consider starting the all // comments with a capital letter. Currently this is not done consistently.	Cosmetic	REJECT

Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveMonitor.java', revision 1179

Lines	Description	Classification	Status
112	Consider synchronizing the method	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/deploy/complete_settings.xml', revision 1179

Lines	Description	Classification	Status
439-440	The name and the type should not be the same.	Cosmetic	REJECT

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FilePreservationState.java', revision 1179

Lines	Description	Classification	Status
130	Consider removing validation. Validation is only required for public methods	Cosmetic	OK
298	Change to: if (referenceCheckSum.isEmpty())	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DerbyServerSpecifics.java', revision 1179

Lines	Description	Classification	Status
General	Missing svn:keywords property with value: URL Revision Author Date Id	Cosmetic	OK
62	1) ArgumentNotValid on the connection. 2) Why do we throw an SQL exception? Usually we catch non-Netarkivet exceptions and throw Netarkivet exceptions instead. 3) Correct the javadoc. Add the connection and throws also ArgumentNotValid. And add the ArgumentNotValid to the method description.	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/common/settings.xml', revision 1179

Lines	Description	Classification	Status
437	better naming. The name and the type should differ	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DerbySpecifics.java', revision 1179

Lines	Description	Classification	Status
General	Missing svn:keywords property with value: URL Revision Author Date Id	Cosmetic	OK
44	javadoc	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/ReplicaCacheDatabase.java', revision 1179

Lines	Description	Classification	Status
General	There are many retrieval methods. Consider inlining some of them.	Cosmetic	POSTPONE
General	redundant logging when IOFailure is thrown at the same time: log.warn(msg); throw new IOFailure(msg, e); Suggest removing the log.warn(msg) statement	Cosmetic	OK
General	Why this instead of just Date now = new Date(); Date now = new Date(Calendar.getInstance().getTimeInMillis());	Cosmetic	OK
72	Describe that the settings are used for initialising the database	Cosmetic	OK
104-105	Strange way for to check for existence or better comment. Why not instead: if (!repIds.contains(rep.getId)) { insert in database } else { repIds.remove(rep.getId()); log.info("");	Cosmetic	OK
119-123	I wonder whether this shouldn't be fatal. Consider throwing an IllegalState in this case.	Minor	OK
127, 133	Consider using the "in" instead of "within"	Cosmetic	OK
132-133	IOFailure is not the correct exception here. It should be IllegalState instead.	Cosmetic	OK
184, 219	Need more info in msg. Write instead: "Cannot add the following replica to the database: " + rep.toString()	NA	OK
228	Change to "The fields for this new entry are set .."	Cosmetic	OK
230	"each replica_id" is confusing. Consider changing from "each replica_id" to "The replica_id for the given replica"	Cosmetic	OK
252	What does this mean?	Cosmetic	OK
295-296	Wrong comment for the statement below (evidently cut'n'pasted from retrieveIdsFromReplicaTable() method)	Cosmetic	OK
310-311	Wrong comment for the statement below (evidently cut'n'pasted from retrieveIdsFromReplicaTable() method)	Cosmetic	OK
320	typo: "no such file_if" => "no such file_id"	Cosmetic	OK
329	Wrong comment for the statements below	Cosmetic	OK
359	typo: "retrieving thie" => retrieving the"	Cosmetic	OK
401	Cut and paste. Change to "sql for retrieving the replicafileinfo_guid for the given fileid"	Cosmetic	OK
425	Change "replica status" => "replica type"	Cosmetic	OK
434	typo: "The if for the file" => "The id for the file"	Cosmetic	OK
457-458	Change to " The SQL statement to retrieve the replica_id for the entry with the given replicafileinfo_guid ..."	Cosmetic	OK
497-498, 502, 507	There is no given entry here!	Cosmetic	OK
705	What does this mean? This is seen in other lines as well	Cosmetic	OK
821, 836	This is the wrong exception to throw here. Use IllegalState instead	Cosmetic	OK
849-850, 859-867	Instead of these findColumn statements, you could change the SELECT statement: SELECT replicafileinfo_guid, replica_id, file_id, segment_id, checksum, upload_status, filelist_status, filelist_checkdatetime, checksum_checkdatetime FROM replicafileinfo WHERE replicafileinfo_guid = ?" and then new ReplicaFileInfo(res.getLong(1), res.getString(2), res.getLong(3), res.getLong(4), res.getString(5), res.getInt(6), res.getInt(7), res.getDate(8), res.getDate(9)));	Cosmetic	OK
898	valid => non-empty	Cosmetic	OK
916-917	Missing argument validation in public method	Cosmetic	OK
1154	missing validation of argument replica	Cosmetic	OK
1209	Missing validation of arg replica	Cosmetic	OK
1216	Add this information to the javadoc	Cosmetic	OK
1236	Missing validation of arg replica	Cosmetic	OK
1244	Add this information to the javadoc	Cosmetic	OK
1264	Missing argumen validation	Cosmetic	OK
1286	Missing argumen validation	Cosmetic	OK
1307	Missing argumen validation	Cosmetic	OK
1327	Missing argumen validation	Cosmetic	OK
1350	Missing argumen validation	Cosmetic	OK
1352-1353	You are checking in the database up against FileListStatus.OK not MISSING. Change text to " for the replica which have the filelist_status set to OK"	Cosmetic	OK
1374	missing validation of argument	Cosmetic	OK
1397-1398	Consider sending a notification about this. This is critical, isn't it?	Minor	OK
1420-1421	Missing argument validation	Cosmetic	OK
1447-1448	Shouldn't we send a notification about this?	Minor	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DBSpecifics.java', revision 1179

Lines	Description	Classification	Status
General	Missing svn:keywords property with value: URL Revision Author Date Id	Cosmetic	OK
54	javadoc	Cosmetic	OK
86	javadoc the connection + permissiondenied	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileBasedActiveBitPreservation.java', revision 1179

Lines

Description

Classification

Status

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DerbyEmbeddedSpecifics.java', revision 1179

Lines	Description	Classification	Status
General	Missing svn:keywords property with value: URL Revision Author Date Id	Cosmetic	OK
84	Javadoc + throw PermissionDenied and IOFailure	Cosmetic	OK