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 |