= Review (NS-125): Assignment B.2.2a: Checksum replica with FileBasedActiveBitPreservation =
|| Author || Jonas ||
|| Moderator || Jonas ||
|| State || Closed ||
== Objectives ==
{{{
The Checksum Replica are finished and it can now be handled by FileBasedActiveBitPreservation.
Also, check whether the unit test concerning the checksum replicas works properly (using test settings).
}}}
'''Total Time Used (Coding,Documentation,Review)''':

{{{
Time use (Coding,Documentation,Review)
HBK: 0.6
JOLF: 6
}}}
'''General comments''':
|| '''Description''' || '''Classification''' || '''Status''' ||
|| Change ArgumentNotValid javadoc to contain the argument type along with the name || Cosmetic || OK ||
|| Add ' throws ArgumentNotValid' after metods throwing this execption. Check that it is in the code documentation. || Cosmetic || OK ||


=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumFileServer.java', revision 1153 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 211-212 || It does not remove the bad entry. It is moved from the archive file to the 'wrong' entry file instead. || Cosmetic || OK ||
|| 222, 226 || Why is it first checked whether an entry for the filename exists after it has been retrieved? The "if(!cs.hasEntry(filename))" could be replaced by "if(currentCs == null)", since currentCs is null if it was not found within the archive. || Minor || OK ||
|| 230 || comment that this exception is caught later in this function and used to give a NotOk reply the correct message.  Do not log here! || Cosmetic || OK ||
|| 236 || String credentialsReceived || Cosmetic || OK ||
|| 243-244 || Do not return and do not set the message to NotOk here. Instead throw an exception, which can be caught later and used properly.  Do not log here. || Cosmetic || OK ||
|| 257-258 || Is this file removed after use? It should. || Minor || OK ||
|| 281 || throws ArgumentNotValid in method header, now that its in java doc. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FilePreservationState.java', revision 1149 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 213-216 || Comment about this! Checks whether the replica has the correct checksum and that the replica is a bitarchive (e.g. that it actually has the file and not just the checksum). || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/JMSArcRepositoryClient.java', revision 1152 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 405-406 || This is completely wrong. This should not be the replica Id, but the replica identification channel name. || Minor || INVALID ||
|| 416 || Insert type in ArguementNotValid || Cosmetic || OK ||
|| 527 || throws after method and javadoc || Cosmetic || OK ||
|| 634-646 || javadoc which exceptions are thrown. ArgumentNotValid and IOFailure. || Cosmetic || OK ||
|| 640 || Not name. The id of the replica. || Cosmetic || OK ||
|| 648 || throws after method and javadoc || Cosmetic || OK ||
|| 665 || remove comment || Cosmetic || OK ||
|| 669 || This could be misunderstood. Not convert to correct message but to the expected message type. || Cosmetic || OK ||
|| 680-681 || log the error message? Handle the error! || Minor || OK ||
|| 705 || throws after method and javadoc || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileBasedActiveBitPreservation.java', revision 1152 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 254 || The list of filenames can potentially be more than 1,200,000 (The last figure I heard of the amount of files within Netarkivet), which fill the log files very fast (e.g. more than 20 MB per replica). Also, it is inefficient to create the final string for each replica.  Restrict to 10 entries in the filenames list. || Minor || OK ||
|| 274-275, 278, 282 || The variable name checksumsForFileOnBa is refering only to BitArchives. Change to checksumForFileOnRep. || Cosmetic || OK ||
|| 295 || Do this! || Minor || OK ||
|| 320 || This TODO has been done. It now collects the checksums the indended way. || Cosmetic || OK ||
|| 334 || through the/a checksum || Cosmetic || OK ||
|| 470 || throws after method and javadoc || Cosmetic || OK ||
|| 472 || The WorkFiles.MISSING_FILES_BA should be changed to WorkFiles.MISSING_FILES_REPLICA. || Cosmetic || REJECT ||
|| 538 || This variable should be renamed to "extraFilesInReplica", since it is not BitArchive specific any more. || Cosmetic || OK ||
|| 652-654 || This does not need 3 lines. || Cosmetic || OK ||
|| 717-720 || Write what is done for the checksum replica. || Minor || OK ||
|| 729 || Not needed in private method || Cosmetic || OK ||
|| 735-737 || Comment about what is done here. || Cosmetic || OK ||
|| 906 || missing '.' || Cosmetic || OK ||
|| 910-912 || This does not need 3 lines. || Cosmetic || OK ||
|| 921 || bad comment. Remove bitarchive. || Cosmetic || OK ||
|| 967 || missing '.' || Cosmetic || OK ||
|| 972 || fewer lines || Cosmetic || OK ||
|| 1026 || throws after method and javadoc || Cosmetic || OK ||
|| 1041-1042 || combine these lines. || Cosmetic || OK ||
|| 1061 || The credentials for correction the bad entry.  Like the other places. || Cosmetic || OK ||
|| 1074 || The variable name arcrep suggests that it is the ArcRepository and not a ArcRepositoryClient. || Cosmetic || OK ||
|| 1157 || throws after method and javadoc || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/distribute/ReplicaClient.java', revision 1152 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || public in front of interface method is give. Can be removed. || Cosmetic || INVALID ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveClient.java', revision 1152 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 216-229 || This method can only be called by the ArcRepository (or ArcRepositoryServer), and the CorrectMessages should only be sent by the BitPreservation. The ArcRepository should then resend the CorrectMessage to this BitarchiveClient, which is done in the following method. Thus this method should not exist. || Minor || OK ||
|| 221 || missing '.' || Cosmetic || INVALID ||
|| 238, 251, 266, 281, 298-299 || Make ArgumentNotValid checks. || Minor || OK ||
|| 239-241 || Handle correct messages ! || Minor || OK ||
|| 253-255 || Handle GetAllFilenamesMessage! || Minor || OK ||
|| 268-270 || Handle GetAllChecksumsMessage! || Minor || OK ||
|| 283-285 || Handle GetChecksumMessage! || Minor || OK ||
|| 300-302 || Be able to create and send GetChecksumMessage based on the channel and filename. || Minor || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/ArcRepositoryServer.java', revision 1152 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 239-252 || Should this function not also cover Bitarchives ? Make Bug! || Minor || OK ||
|| 258-271 || As above. Checksum only. Make Bug! || Minor || OK ||
|| 307-313 || Write that this currently only works for ChecksumArchive. Also, make it work for bitarchives. Make Bug that it does not yet work for bitarchive replicas || Minor || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/GetChecksumMessage.java', revision 1148 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 62 || Isn't second argument only parameter name? || Cosmetic || REJECT ||
|| 123 || Only name of variable || Cosmetic || REJECT ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/CorrectMessage.java', revision 1152 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 69 || throws after method and javadoc || Cosmetic || OK ||
|| 96 || throws after method and javadoc Type on arguement || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/FileChecksumArchive.java', revision 1153 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 236 || maybe change to, "create file if checksumFile do not exists." || Cosmetic || OK ||
|| 271 || comment on synchronized like below. || Cosmetic || OK ||
|| 316 || missing '.' || Cosmetic || REJECT ||
|| 466, 469 || We generally do not use IOException. Instead catch and throw new IOFailure. || Minor || OK ||
|| 491 || It does not use the double hash for separating the data and the wrong record. It uses colon surrounded by spaces instead. || Cosmetic || OK ||
|| 520-521 || Implement TODO! || Minor ||REJECT ||
|| 550 || Yes, that would properly be best. || Minor || OK ||
|| 663 || Remove TODO and return sounds right. || Cosmetic || OK ||
|| 664 || rewrite! || Cosmetic || OK ||
|| 691 || Perhaps the checksumFile should be synchronized with the archive in memory? || Minor || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/GetAllFilenamesMessage.java', revision 1151 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 109 || javadoc || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/LocalArcRepositoryClient.java', revision 1145 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 361-364 || javadoc! Also, Implement this function.  Check whether this is called! and handle this. || Minor || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/ArcRepository.java', revision 1151 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 554 || valid xhtml <br/>? || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumClient.java', revision 1152 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 94 || throws after method and javadoc || Cosmetic || OK ||
|| 104-132 || This function is invalid and should be removed. The correct message should only be created by the bitpreservation and resend which is the case in the following method. || Minor || OK ||
|| 134 || javadoc || Cosmetic || OK ||
|| 135 || throws after method and javadoc || Cosmetic || OK ||
|| 141, 157, 172, 189, 245 || The resending log messages should not contain newline characters. The log message in line 214 is much better. || Cosmetic || OK ||
|| 205 || throws after method and javadoc || Cosmetic || OK ||
|| 207 || Should this be implemented now? || Minor || REJECT ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/AdminDataMessage.java', revision 1150 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 91 || Java doc beside @return || Cosmetic || OK ||
|| 128 || Java doc beside @return || Cosmetic || OK ||