= Review (NS-147): Archive Assignment B.2.2 complete =
|| Author || Jonas ||
|| Moderator || Jonas ||
|| State || Closed ||
== Objectives ==
{{{
Besides containing all the latest work on database archival, it also contains a lot of code style changes.
}}}
== Summary ==
{{{
Follow up by JOLF
}}}
'''Total Time Used (Coding,Documentation,Review)''':

{{{
Time use (Coding,Documentation,Review)
JOLF: 8 MD
SVC: 2 MD
}}}
'''General comments''':
|| '''Description''' || '''Classification''' || '''Status''' ||
|| Java logging is used a couple of places in the Deploy code. And we're not running DeployApplication with a log.properties assigned to it. Consider using the same log.prop when running the DeployApplication, so the any DeployApplication logging is written to logs:  -Dorg.apache.commons.logging.Log=org.apache.commons.logging.impl.Jdk14Logger -Djava.util.logging.config.file=/home/test/SVC/conf/log_ArcRepositoryApplication.prop || Cosmetic || POSTPONE ||
|| Our use of the word arcrepository indicates the storage of arcfiles. Consider renaming "arcrepository" as "repository" || Cosmetic || POSTPONE ||


=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumFileServer.java', revision 1320 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 161 || make "_" as a constant || Cosmetic || OK ||
|| 180 || Spelling: Receving => Receiving || Cosmetic || OK ||
|| 300 || Spelling: Recieving => Receiving || Cosmetic || OK ||
|| 338 || Spelling: Receving => Receiving || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/ReplicaFileInfo.java', revision 1283 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 72 || Add validation: fDate/cDate (not null), the String values (not null not empty), the int/long values (positive integers) || Cosmetic || OK ||
|| 164 || rename method: getFileListCheckdatetime() || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveMonitor.java', revision 1283 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 237 || That depends on the semantics, we want to impose: If exceptions is null means no exceptions, null is ok; otherwise we should disallow null; and enforce an empty list meaning no exceptions.  In anycase the javadoc for BitarchiveReply should describe our chosen semantics || Cosmetic || INVALID ||
|| 312 || spelling: live => alive || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/ReplicaCacheDatabase.java', revision 1311 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 125 || ID => identifiers || Cosmetic || OK ||
|| 348 || Add to logentry and to javadoc: Returning the first one || Cosmetic || OK ||
|| 378 || Add: Returning the first one || Cosmetic || OK ||
|| 877 || Implement TODO, or remove TODO || Cosmetic || OK ||
|| 1386 || Dangerous use of ==. May or not work as intended Use instead if (us.equals(ReplicaStoreState.UPLOAD_COMPLETED)) || Cosmetic || OK ||
|| 1788 || This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc || Cosmetic || OK ||
|| 1821 || This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc || Cosmetic || OK ||
|| 1989 || spelling: bitarche => bitarchive || Cosmetic || OK ||
|| 2061 || Missing validation of argument 'line' || Cosmetic || OK ||
|| 2079-2080 || Remove? || Cosmetic || OK ||
|| 2132 || Add null validation of 'date' argument || Cosmetic || OK ||
|| 2160 || Can we kill this method now, or should we wait to the next stable release? No, just kill FIXME comment! || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/settings.xml', revision 1300 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/JMSArcRepositoryClient.java', revision 1295 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 537 || Spelling: Recieved => Received || Cosmetic || OK ||
|| 597 || Spelling: Recieved => Received || Cosmetic || OK ||
|| 657 || Spelling: Recieved => Received || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/LinuxMachine.java', revision 1305 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 1316-1318 || Explain, that the output of shutdown is extended to the same file as the output when starting the database. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/tools/ReestablishAdminDatabase.java', revision 1311 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || missing SVN:keywords || Cosmetic || OK ||
|| 145 || Make into local constant or include with dk.netarkivet.archive.Constants || Cosmetic || OK ||
|| 165 || spelling: occured => occurred || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/ScriptConstants.java', revision 1305 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 70 || Please rename to OPTION_SECURITY_POLICY || Cosmetic || OK ||
|| 72 || Please rename to OPTION_SECURITY_POLICY_WIN || Cosmetic || OK ||
|| 74 || This is not an appropriate description. || Cosmetic || OK ||
|| 81-82 || Change to one line || Cosmetic || OK ||
|| 84-85 || Change to one line || Cosmetic || OK ||
|| 118 || change to ETC_PROFILE || Cosmetic || OK ||
|| 125-128 || Replace with calls to StringUtils.repeat || Cosmetic || REJECT ||
|| 356, 369, 382, 398, 414 || These checkNotNulls should be replaced with checkNotNullOrEmpty || Cosmetic || OK ||
|| 429, 447-449 || These checkNotNulls should be replaced with checkNotNullOrEmpty || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BatchEndedMessage.java', revision 1289 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 84 || replace 'to' with 'ChannelID to'. || Cosmetic || OK ||
|| 85-86 || replace 'baAppId' with 'String baAppId' || Cosmetic || OK ||
|| 88 || replace 'originatingBatchMsgId' with 'String originatingBatchMsgId' || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/UpdateableAdminData.java', revision 1283 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || There are multiple arguments in this class named "arcfileName". Rename as "fileName" || Cosmetic || OK ||
|| General || All references til "arcrepository" should maybe be just "repository" || Cosmetic || POSTPONE ||
|| 106 || spelling: "ddmin data" => "admin data" || Cosmetic || OK ||
|| 109, 191, 195 || This is used in AdminData.toString method to include there information about known bitarchives. The knownBitarchives could probably now be replaced by a database call || Cosmetic || REJECT ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveClient.java', revision 1298 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 173 || Removal initial \n || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/Machine.java', revision 1302 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 94, 103 || rename "bpdbFileName" as "arcdatabaseFilename" || Cosmetic || OK ||
|| 681 || Rename JMXREMOTE_HERITRIX_PRIVELEGES as JMXREMOTE_HERITRIX_PRIVILEGES || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/CDXOriginCrawlLogIterator.java', revision 1289 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 162 || Spelling: matchin=> matching || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/WindowsMachine.java', revision 1302 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 33 || Why has this been added, when it is not used? || Cosmetic || OK ||
|| 46 || rename 'e' as 'root' || Cosmetic || OK ||
|| 868-869 || This indentation looks quite odd. Insert line break. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/FileChecksumArchive.java', revision 1320 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 123 || Maybe, you should rather write: map (filename -> checksum) || Cosmetic || OK ||
|| 140 || Consider adding 'synchronized' to this method || Cosmetic || OK ||
|| 341-346 || Test whether the file is readable || Cosmetic || OK ||
|| 360 || make as a constant || Cosmetic || OK ||
|| 400 || Spelling: occured => occurred || Cosmetic || OK ||
|| 529-550, 553-574 || Duplicate code here; Have only one method and return a dk.netarkivet.common.utils.KeyValuePair instead || Cosmetic || OK ||
|| 605 || spelling: occured => occurred || Cosmetic || OK ||
|| 645-657 || Replace 'arcFile' with 'file'; this code accepts (and should accept) any kind of files || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DatabaseAdmin.java', revision 1285 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Add missing svn:keywords || Cosmetic || OK ||
|| 67 || Add "synchronized" to the method to prevent synchronization problems || Cosmetic || OK ||
|| 117 || add: uniquely identifying the replica || Cosmetic || OK ||
|| 141-142 || Is this still an outstanding issue? Add FR to handle this issue about storestate validity || Cosmetic || REJECT ||
|| 295-296 || Make a more meaningful text for this exception || Cosmetic || OK ||
|| 346-348 || Can be replaced with just instance = null; || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/CombiningMultiFileBasedCache.java', revision 1283 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 45-46 || Or instead of "Inheriting the ": Must implement the java.lang.Comparable interface || NA || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/AdminFactory.java', revision 1283 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || add SVN:keywords || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/LocalArcRepositoryClient.java', revision 1321 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 423 || Add @throws PermissionDenied if the credentials for correcting files are wrong || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DatabaseBasedActiveBitPreservation.java', revision 1293 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 221 || Clarify your TODO. This is also about if or when the checksum information from the replica expires, isn't it? || Cosmetic ||OK ||
|| 436 || Add: valid meaning not null and not empty || Cosmetic || OK ||
|| 463 || add: invalid meaning not null; Should we accept not empty filenames here? || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/RemoveAndGetFileMessage.java', revision 1283 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 45, 61, 67, 70 || Rename as "filename" || Cosmetic || OK ||
|| 138-140 || Rename as getFilename() || Cosmetic || OK ||
|| 182 || Arcfile => File || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/WorkFiles.java', revision 1288 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 254 || Add "." after "below here" || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumClient.java', revision 1298 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 95 || Wrong comment: A new instance is now created every time || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/CreateTestInstance.java', revision 1301 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 139 || Change constant COMPLETE_HARVEST_HETRIX_GUI_PORT to COMPLETE_HARVEST_HERITRIX_GUI_PORT || Cosmetic || OK ||
|| 141 || Change constant COMPLETE_HARVEST_HETRIX_JMX_PORT to COMPLETE_HARVEST_HERITRIX_JMX_PORT || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/distribute/IndexRequestServer.java', revision 1296 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 199 || Remove line || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/tools/RunBatch.java', revision 1289 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 246-249 || Consider removing this comment, as this is not a priority to HelpFormatter instead. And furthermore I don't understand the problem statement "HelpFormater only prints directly". It doesn't seem to be true. If it is a problem with cli-1.0 then it could be solved in the latest cli-release (1.2; released march 19, 2009) || NA || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FilePreservationState.java', revision 1294 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || The mention of "bitarchives" should be replaced with replica. This is both an issue regarding javadoc, method-naming, and argument naming (i.e. Replica bitarchive in method getBitarchiveChecksum()) || Cosmetic || OK ||
|| 73 || We're not validating the checksumMap, here. Is that on purpose? Validate for null and not empty map || Cosmetic || OK ||
|| 107 || The localisation should probably take place in the ReplicaStoreState class.  This is possible if the getAdminBitarchiveStoreState does not return null in the case of "No state", but instead returns a ReplicaStoreState object representing "No state" || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/FileBasedCache.java', revision 1289 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 51 || Spelling: The type of cache || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileListJob.java', revision 1289 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/webinterface/BitpreserveFileState.java', revision 1290 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 86 || Consider adding a log.warn message at this point || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DerbySpecifics.java', revision 1285 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 32 || Add to javadoc, why this class is so short! || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/FileListStatus.java', revision 1276 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 30 || => This is used by .. || Cosmetic || OK ||
|| 43-44 || certain => specific || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveMonitorServer.java', revision 1283 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Consider adding validation for the visit() methods in this class || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileBasedActiveBitPreservation.java', revision 1283 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 962 || Remove TODO; as we don't intend to implement this method || Cosmetic || OK ||
|| 975 || Remove TODO; as we don't intend to implement this method || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/deploy/Constants.java', revision 1304 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 168, 171, 181 || HETRIX => HERITRIX || Cosmetic || OK ||
|| 325 || Rename file? bpdb.jar refers to a bitpreservation database file, and the database is now much more. || Cosmetic || OK ||
|| 518 || Archive database ... || Minor || OK ||
|| 577, 590 || Be more strict: Disallow empty strings as valid scopes || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/ArcRepositoryServer.java', revision 1298 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 241 || Is this still true? If not, remove line || Cosmetic || OK ||
|| 255 || Change to "Failed to handle GetAllFilenamesMessage: " ... || Cosmetic || OK ||
|| 276 || Change to "Failed to handle GetAllChecksumsMessage: " ... || Cosmetic || OK ||
|| 292 || Spelling: Recieved => Received || Cosmetic || OK ||
|| 319 || Is this still true? If not remove TODO line || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/RawMetadataCache.java', revision 1310 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 180 || Add: Currently does nothing || Cosmetic || OK ||
|| 185-191, 205 || throws IOFailure || Cosmetic || OK ||
|| 206-210 || delete? || Cosmetic || OK ||
|| 216 || Add: Currently does nothing || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DerbyServerSpecifics.java', revision 1283 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 44-49 || Javadoc is misleading; Change to: Method for shutting down the database system. Has no effect in this implementation Maybe log a warning about this fact when the method is called. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DatabasePreservationState.java', revision 1283 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || missing license header || Cosmetic || OK ||
|| General || Replace all(?) instances of "bitarchive" with "replica" in this class || Cosmetic || OK ||
|| General || Consider adding a featurerequest for some of TODOs || Cosmetic || OK ||
|| General || Consider replacing == with equals, if not certain that they have equal semantics || Cosmetic || OK ||
|| 42 || rename "file" as "filename" || Cosmetic || OK ||
|| 92-93 || Is this correct behaviour and if yes, document it in the javadoc. || Cosmetic || OK ||
|| 98 || If this is for presentation purposes only, move it to the archive.webinterface package || Cosmetic || OK ||
|| 134-136 || Add to javadoc, that it always returns  true || Cosmetic || OK ||
|| 158 || Complete logmessage || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/ArcRepository.java', revision 1298 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 211-217 || why make a variable which is only used once? Make the string in the creating of the exception. || Cosmetic || OK ||
|| 396 || Refactor to be a method inside one of the Admin implementations || Cosmetic || POSTPONE ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DBConnect.java', revision 1300 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 114 || Will this only work for an external database? If no update javadoc. || Cosmetic || OK ||
=== Comments on file 'trunk/webpages/BitPreservation/Bitpreservation-filestatus-checksum.jsp', revision 1288 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 126-129 || Too long lines || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/Admin.java', revision 1283 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Add svn:keywords || Cosmetic || OK ||
|| 45 || add newline || Cosmetic || OK ||
|| 61 || add newline || Cosmetic || OK ||
|| 64-65 || Deprecate this method; as in the future it will not be possible to setChecksum || Cosmetic || OK ||
|| 79 || add newline || Cosmetic || OK ||
|| 86 || add newline || Cosmetic || OK ||
|| 104 || add newline || Cosmetic || OK ||
|| 108 || This is not decided in the interface, but in the actual implementation(s) || Cosmetic || OK ||
|| 116 || add newline || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/ArchiveStoreState.java', revision 1283 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 74 || throws ArgumentNotValid! || Cosmetic || OK ||
|| 90 || throws ArgumentNotValid || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/PreservationState.java', revision 1283 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Add svn:keywords || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveServer.java', revision 1283 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Consider adding validation for the visit methods in this class || Cosmetic || OK ||