= Review (NS-108): Review of current changes caused by Assignment B2.2 (since last review) =
|| Author || Jonas ||
|| Moderator || Jonas ||
|| State || Closed ||
== Objectives ==
{{{
The Checksum replicas are not able to be handled by bitpreservation yet, and it is therefore not as important that it works.
It has to be ensured that the changes does not cause problems for the bitarchive replicas.
Some changes for improving the general structure was found in the last review (NS-88). These are described in the Assignment description.
}}}
'''Total Time Used (Coding,Documentation,Review)''':

{{{
Time use (Coding,Documentation,Review)
JOLF: 2.5
SVC: 0.25
}}}
'''General comments''':
|| '''Description''' || '''Classification''' || '''Status''' ||


=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumFileServer.java', revision 1080 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 178-185, 217-224 || This should be changed to fit the changes in the CorrectMessage. The RemoteFile should not be retrievable, since it has to be maintained that it is cleaned up afterwards. Instead have a method in CorrectMessage where the file is retrieved and the RemoteFile is cleaned up.  This has to be implemented consistenly with the other message classes || Minor || OK ||
|| 215 || [spelling]: receving => receiving || Cosmetic || OK ||
|| 253-254 || Describe that this exception can be thrown. || Cosmetic || Reject ||
=== Comments on file 'trunk/src/dk/netarkivet/common/distribute/Channels.java', revision 1051 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || There need to be a redesign of the channels for the replicas. Only the BitarchiveMonitors should know and use the ALL_BA and ANY_BA channels, and the channels THE_BAMON and THE_CR (new checksum channel) should be replaced by a single channel for each replica, e.g. a replica channel. Though this problem cannot be handled as part of this review. It should instead be handled later by Assignment B2.2. || Cosmetic || OK ||
|| 105, 133-134 || Describe that a 'UnknownID' exception can be thrown. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/JMSArcRepositoryClient.java', revision 1082 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 410 || Is this really the 'replicaId' and not the 'replica identification channel name'? || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/ArchiveSettings.java', revision 1057 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 176-177 || This does not concern the value, only the path to the value within the settings. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/distribute/ReplicaClient.java', revision 1082 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 140 || [language] correcting the wrong file => correcting the corrupt file || Cosmetic || OK ||
|| 144 || Is this correct. Isn't this the new checksum?  Better variable naming required. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileBasedActiveBitPreservation.java', revision 1082 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 295-296 || Remove ":" after TODO. Violates checkstyle || Cosmetic || OK ||
|| 296 || It should now handled checksum replicas! There are at least a method which should do it. || Cosmetic || OK ||
|| 307-313 || Describe what is done here! || Cosmetic || OK ||
|| 312 || It should say 'unhandled replica type' instead. || Cosmetic || OK ||
|| 318-320 || This java-doc should be removed. It is implemented now! || Cosmetic || OK ||
|| 322-323 || implemented the intended method (using the word correct can be confusing in this case). || Cosmetic || OK ||
|| 345-346 || Is this necessary? Two entries for the same file should be treated the same way as for the bitarchive replica. || Cosmetic || OK ||
|| 348-350 || Describe what is done here || Cosmetic || OK ||
|| 355 || handle or describe why not. In any case log the error. || Minor || OK ||
|| 362 || Not just any batchjob. It sends a ChecksumJob which is limitted to some specific files. || Cosmetic || OK ||
|| 518 || not replcia! it is called replica || Cosmetic || OK ||
|| 727-728 || IS there a corresponding log: Bit integrity check completed ? || Cosmetic || OK ||
|| 734 || It is not described in the method definition or java-doc that this exception can be thrown. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveClient.java', revision 1079 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 217, 223, 230, 243 || Remove these lines; and add to method javadoc that they are not yet implemented, and throws NotImplementedExceptions || Cosmetic || OK ||
|| 218, 224, 231, 244 || Instead of "TODO: implemented.." write "Will be implemented in release 3.12.0" || Cosmetic || OK ||
|| 259 || Remove line || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/GetChecksumMessage.java', revision 1054 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 43-44 || Handle this todo. Use the replica id for something. Remove! || Cosmetic || OK ||
|| 58 || Does the super method validate to and replyTo? In that case the validation of those fields are not needed here. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/GetAllChecksumsMessage.java', revision 1078 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 46 || Bad variable naming || Cosmetic || OK ||
|| 71 || Shouldn't this method  be named setResultingFile || Cosmetic || OK ||
|| 78 || Add to javadoc: This method can only be called once, because it deletes the remoteFile, and sets the remoteFile field to null. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/CorrectMessage.java', revision 1054 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 26 || Remove this unused import. || Cosmetic || OK ||
|| 81-87 || Is is not a better idea to retrieve the file and then secure that the file is cleaned up afterwards, than making it possible to retrieve the RemoteFile and then have the retriever do the cleanup? This message has to be consistent with the other message classes containing remote files. || Minor || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/FileChecksumArchive.java', revision 1054 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 102 || Is this comment usefull? I think not || Cosmetic || OK ||
|| 193-195 || Maybe it should be if (checksumFile.length * 2 > FileUtils.getBytesFree(..)) to be on the safe side.  Perhaps 'if(checksumFile.length + minimum_space_left > ...) || Cosmetic || OK ||
|| 241 || Describe why this is called if the file already exists. || Cosmetic || OK ||
|| 245-278 || If an entry is incorrect, then it is placed in the 'wrongEntryFile', but it is not removed from the checksum file. Untill the checksum file is recreated (by a correct message), then every time the ChecksumArchive is started, a copy of a bad entry will then be placed in the 'wrongEntryFile', which will be seen as spam. Thus this fuction should tell whether a wrong entry was found and if that is the case then the checksum file should be recreated. || Cosmetic || OK ||
|| 281-285 || Reformulate javadoc. What do you mention "the correct archive file". The name of the method is saveArchiveFile. Maybe rename method || Cosmetic || OK ||
|| 451 || describe why this has to be synchronized around the checksum file. || Cosmetic || OK ||
|| 506 || calculate the checksum from the file instead. || Cosmetic || OK ||
|| 547 || it does not throw IOFailure any more || Cosmetic || OK ||
|| 599 || It should not be a RemoteFile but instead just a file. || Cosmetic || OK ||
|| 608-610 || ARGUMENT NOT VALID? || Cosmetic || OK ||
|| 632-633 || Use the method in ChecksumJob for making a checksum entry string ('makeLine'). || Cosmetic || OK ||
|| 636 || as above || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/ArcRepository.java', revision 1053 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 173-174 || This should be replaced by 'Replica.getKnown()'. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/ChecksumArchive.java', revision 1054 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 73 || Use file instead of RemoteFile. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumClient.java', revision 1078 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 192 || Remove ":" after TODO. Violates checkstyle || Cosmetic || OK ||
|| 199 || Remove initial Linebreak: "\n" in logmessage || Cosmetic || OK ||
|| 210 || This doesn't make any sense for me || Cosmetic || OK ||
|| 218-219 || Remove linebreak after "rf" || Cosmetic || OK ||
|| 245 || Missing argument validation || Cosmetic || OK ||
|| 265 || Missing argument validation || Cosmetic || OK ||
|| 281 || Missing argument validation || Cosmetic || OK ||
|| 298 || Missing argument validation || Cosmetic || OK ||
|| 315 || Missing argument Validation || Cosmetic || OK ||