= Review (NS-88): B.2.2 changes =
|| Author || Jonas ||
|| Moderator || Jonas ||
|| State || Review ||
== Objectives ==
{{{
The checksum archive is not properly handled by the FilebasedActiveBitPreservation.
This does not handle the new files for B.2.2b (The new files in dk.netarkivet.archive.bitpreservation).
Please ensure that the FilebasedActiveBitPreservation is not called directly any more. Instead the ActiveBitPreservationFactory should be used for initializing any instance of ActiveBitPreservation.
}}}
'''Total Time Used (Coding,Documentation,Review)''':

{{{
Time use (Coding,Documentation,Review)
KFC: 2.0
JOLF: 20.0
dk.netarkivet.common.utils.DBUtils + dk.netarkivet.common.distribute.arcrepository.TrivialArcRepositoryClient -> Follow up by KFC
The rest -> Follow up by JOLF
}}}
'''General comments''':
|| '''Description''' || '''Classification''' || '''Status''' ||
|| dk.netarkivet.archive.webinterface.BitpreserveFileState uses FilebasedActiveBitPreservation directly 4 times. || Minor || OK ||


=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumFileServer.java', revision 1006 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 39 || Java-doc the class || Cosmetic || OK ||
|| 45 || No reason for "...getName()" || Cosmetic || OK ||
|| 82 || This has been decided! A ChecksumReplica use a THE_CR channel. || Cosmetic || OK ||
|| 88 || I think not. It is only used to know which archives to wait for batch replies from || Cosmetic || OK ||
|| 94 || The checksum archive is defined in the ChecksumServerAPI interface. This should at least be described somewhere. Put 'cs' in this class instead. || Minor || OK ||
|| 117 || remove || Cosmetic || OK ||
|| 127 || X (and the others) || Cosmetic || OK ||
|| 194 || Not a record, an arc-file entry. In this case a line in the file of the FileChecksumArchive. || Cosmetic || OK ||
|| 247 || Unnecessary emtpy line || Cosmetic || OK ||
|| 250 || A very poor warning message. Could be other errors/exceptions. Put 'msg' into the log. || Cosmetic || OK ||
|| 272 || Put 'msg' into the log. || Cosmetic || OK ||
|| 289 || Spelling error: 'recieving', not 'receving'. Also make 'get all checksum message' into the name of the message.  NO: receiving! || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/settings.xml', revision 940 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 59 || This class needs to be moved to common, now || Minor || Postpone ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumServerAPI.java', revision 937 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 13 || Perhaps the name should be ChecksumArchiveServerAPI (if API is retained)? || Cosmetic || OK ||
|| 13 || Could this share a common superinterface with BitArchiveServer? || Cosmetic || Postpone ||
|| 13 || API is usually considered bad practice in interface names || Cosmetic || OK ||
|| 16-19 || REMOVE || Minor || OK ||
|| 30-41 || java-doc || Cosmetic || OK ||
|| 36-37 || It is unclear to me that these need to be part of the public API || Minor || OK ||
|| 43-47 || remove! || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/GetAllChecksumMessage.java', revision 992 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 39-40 || The output is a file, which corresponds to the reply file from a ChecksumJob. || Cosmetic || OK ||
|| 39 || Should it say "all the files?" || Cosmetic || OK ||
|| 44 || Shouldn't it be "GetAllChecksumsMessage || Cosmetic || OK ||
|| 46 || As above: Don't use || Cosmetic || OK ||
|| 118 || Why implement it if only calls super.toString()? || Cosmetic || OK ||
|| 121 || Write java-doc, or add @Override. Or both?  Just java-doc. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/JMSArcRepositoryClient.java', revision 1006 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 505 || I strongly dislike that we have different methods for doing this on bitreplicas and checksumreplicas. It seems to break the abstraction of bitarchives badly. || Minor || Postpone ||
|| 522-523, 537, 539-543 || We should generify the Synchronizer to be Synchronizer<T extends NetarkivetMessage> and handle the classcasting in there || Cosmetic || Postpone ||
|| 532-533 || That's not an empty list or indeed file. That's null. Update javadoc, if null is possible || Cosmetic || OK ||
|| 547 || This should be placed in the tempDir from the settings: 'settings.common.tempDir'. || Cosmetic || OK ||
|| 552-553 || Remove double space (end of first line and first of second line). || Cosmetic || OK ||
|| 563 || As above || Minor || Postpone ||
|| 571 || Would it be possible to extract a supermethod for these two? It seems like a lot of duplicate code. Possibly for batch jobs too || Cosmetic || Postpone ||
|| 605 || This should be placed in the tempDir from the settings: 'settings.common.tempDir'. || Cosmetic || OK ||
|| 618-619 || Comment this function!  checksum must be the 'wrong checksum' of the bad file. || Cosmetic || OK ||
|| 626-629 || What's the relationship between "remove and get one file" and "correct"? Should "remove and get one file" be deprecated (I would like that, that was the original design)? || Minor || Postpone ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveClient.java', revision 959 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Please do not destroy our file header formatting. || Cosmetic || OK ||
|| 43 || Needs a '.' at the end. || Cosmetic || OK ||
|| 48 || No need for ".getName()" || Cosmetic || OK ||
|| 185-186 || Why is there no end paranthesis? || Cosmetic || OK ||
|| 214-257 || Handle these checksum replica messages. The current handling is not appropriate.  Change the handling of these methods || Minor || Postpone ||
|| 260 || No reason to write "Method for..." in javadoc, that's implicit. Javadoc should describe how the method can be used. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/TrivialArcRepositoryClient.java', revision 937 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || I think this class needs to be deprecated or moved to test. LocalArcRepositoryClient does a much more sensible job, and they seem to have the same aim || Minor || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/FileChecksumArchive.java', revision 937 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Make a 'removedChecksum' file, which contains the wrong checksum entries from the correct method. || Minor || Postpone ||
|| 51 || Is this really a good idea? I know the checksumjobs already do this, but I feel unfomfortable with the fact that we can probably break the archive by uploading a file with a "##" in it's name. Make a FR to do something better || Cosmetic || OK ||
|| 65 || .getName is unneccessary || Cosmetic || OK ||
|| 91 || Perhaps use the word "singleton"? || Cosmetic || OK ||
|| 105 || What does it do? Especially: It actually seems to throw exceptions || Cosmetic || OK ||
|| 110-119 || The FileChecksumArchive checks for minimum space left, just like the Bitarchive. Is this even necessary, when each entry has a size smaller than 100 bytes? At least do not use the minimum space left from the Bitarchives. || Minor || Postpone ||
|| 124 || This file should be placed in a archive directory. Not the installation folder.  Make setting for folder name, and make checksum file name contain the replica id. || Minor || Postpone ||
|| 174-182 || 'after all' ... What should that mean in this context? And change to 'file.isFile()'. || Cosmetic || OK ||
|| 217 || seconf? change to 'second'. || Cosmetic || OK ||
|| 219 || Eeeeek! Possible IndexOutOfBounds exception || Minor || OK ||
|| 229 || WRONG. IOFailure only if problems with the checksum file. || Cosmetic || OK ||
|| 231 || This method seems horribly, horribly inefficient. We have room for the current archive in memory, so use that method for now, and remember to use a journal to persist the checksum archive || Minor || Postpone ||
|| 290 || Eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeek! This method is unsynchronized and writes to a file, _and_ it is called from a threaded context (by JMS) || Major || Postpone ||
|| 300 || What to do if the archive entry already exists?  Should the upload be considered a success if they have the same checksum? And a failure if that is not the case? It is a success! || Cosmetic || OK ||
|| 307 || Describe why the 'true' is given as an argument. It means 'append' instead of override. || Cosmetic || OK ||
|| 317-320 || Why not add a newline to the entry we write instead? || Cosmetic || OK ||
|| 325-326 || indentation || Cosmetic || OK ||
|| 377, 396 || Something tells me we have a discepancy here - the MD5 utility method should probably both throw IOFailure, in which case these methods seem unneccessary? || Cosmetic || Postpone ||
|| 415-474 || This function has not a good way of handling the file. If the file is too large, then there will occur memory problems. Perhaps creating a new file with the same content, but where the bad entry has been removed. And then removing the old file and moving the 'new and corrected' file to the correct location in the file system.  Change to a 'correct' method. And synchronize. || Major || Postpone ||
|| 415-416 || This method seems to irrevocably remove stuff from the checksum archive, with no way to even manually restore it. That is very scary. We should probably store the removed checksum somewhere as a record (the log is not a proper place, it is recycled too often) || Minor || Postpone ||
|| 416 || Eeek! Again a method that writes to the file without synchronizing || Major || Postpone ||
|| 449 || If this write fails we loose the entire archive. We need to write to a new file, and move on top of the old one || Major || Postpone ||
|| 464 || I'm more than a little scared that the file is not closed in a finally block || Minor || Postpone ||
|| 486 || Place the file in the tempDir according to the settings: settings.common.tempDir; || Cosmetic || OK ||
|| 488 || Why copy the file? There is a method to make a remote file that doesn't erase the original. Think of synchronization while copying || Cosmetic || Postpone ||
|| 492, 525 || No message? || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/LocalArcRepositoryClient.java', revision 937 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 342-359 || Implement! || Minor || Postpone ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/GetAllFilenamesMessage.java', revision 1006 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 39-40 || Write that this is the Checksum replica equivelent to the batchjob FilelistJob. || Cosmetic || OK ||
|| 43-44 || remove. We do not use uid || Cosmetic || OK ||
|| 85 || No message ? || Cosmetic || OK ||
|| 110-113 || That doc doesn't really make sense in the implementation || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumClient.java', revision 992 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 52-66 || This should be java-doc, not just comment! || Cosmetic || OK ||
|| 64 || Please, no _ in field names || Minor || OK ||
|| 66 || Consider removing this field || Cosmetic || OK ||
|| 71, 73, 87, 89 || These aren't the parameter actually in the method || Cosmetic || OK ||
|| 71, 73, 78 || No _ in param names, please || Cosmetic || OK ||
|| 87, 89, 95 || As above || Cosmetic || OK ||
|| 113 || What about checksum param || Cosmetic || OK ||
|| 115 || Not used || Cosmetic || OK ||
|| 116 || A previous design decision, which I prefer, is that you cannot correct an old file, without knowing the previous checksum, which you are replacing. || Minor || OK ||
|| 136 || X || Cosmetic || OK ||
|| 149 || It should be 'through' not 'though'. || Cosmetic || OK ||
|| 168 || X || Cosmetic || OK ||
|| 186 || X || Cosmetic || OK ||
|| 187-188 || Java-doc what is returned || Cosmetic || OK ||
|| 208 || X || Cosmetic || OK ||
|| 221 || X || Cosmetic || OK ||
|| 234 || X || Cosmetic || OK ||
|| 235, 245, 254, 263, 272 || DOC! || Cosmetic || OK ||
|| 239 || remove auto comment. || Cosmetic || OK ||
|| 240-241 || NotImplemented seems to indicate that it will be implemented later. Is IllegalState better? || Cosmetic || OK ||
|| 248 || remove auto comment. || Cosmetic || OK ||
|| 257 || remove auto comment. || Cosmetic || OK ||
|| 266 || remove auto comment. || Cosmetic || OK ||
|| 272 || It seems to me this should be removed in favour of correct(...) || Cosmetic || Postpone ||
|| 275 || remove auto comment. || Cosmetic || OK ||
|| 282 || remove auto comment. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/ActiveBitPreservationFactory.java', revision 940 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 9-12 || Wouldn't it be better just to name the interface that must be used, and let people find implementing classes from that? This is bound not to be kept updated || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/common/utils/DBUtils.java', revision 937 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 90 || Perhaps it should be updated to take database as parameter? || Minor || OK ||
|| 313 || Again, that must be deprecated, or this method cannot be in the common module || Minor || OK ||
|| 935 || -do- || Minor || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/common/distribute/Channels.java', revision 1006 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 100 || This one looks a little scary - it seems to indicate that order of the list is significant, and somehow relates to order of other lists. That seems to indicate that we need a better modelling. || Minor || Postpone ||
|| 126, 134 || We do not want any nulls in our system. Change to the Checksum replica identification channel!  Remodel! || Minor || Postpone ||
|| 139, 147 || We do not want any nulls in our system. Change to the Checksum replica identification channel!  Remodel! || Minor || Postpone ||
|| 152, 160 || We do not want any nulls in our system. Change to the Checksum replica identification channel!  Remodel! || Minor || Postpone ||
|| 165, 173 || We do not want any nulls in our system. Change to the Bitarchive replica identification channel!  Remodel! || Minor || Postpone ||
|| 277, 282-289 || Handle this differently. The ChannelID should not be able to be 'null', but if the current replica in UseReplicaId is a checksum replica, then the channel in THE_BAMON will be the identification channel for the checksum replica, e.g. THE_CR. If this method is called and the channel refers to the checksum replica identification channel, then a warning should be issued. || Minor || Postpone ||
|| 319, 324-327 || Handle this differently. The ChannelID should not be able to be 'null', but if the current replica in UseReplicaId is a bitarchive replica, then the channel in THE_CR will be the identification channel for the bitarchive replica, e.g. THE_BAMON. If this method is called and the channel refers to the bitarchive replica identification channel, then a warning should be issued.  Remodel! || Minor || Postpone ||
|| 358, 363-366 || Handle this differently. The ChannelID should not be able to be 'null', but if the current replica in UseReplicaId is a checksum replica, then the channel in ALL_BA will be the identification channel for the checksum replica, e.g. THE_CR. If this method is called and the channel refers to the checksum replica identification channel, then a warning should be issued.  Remodel! || Minor || Postpone ||
|| 399, 404-407 || Handle this differently. The ChannelID should not be able to be 'null', but if the current replica in UseReplicaId is a checksum replica, then the channel in ANY_BA will be the identification channel for the checksum replica, e.g. THE_CR. If this method is called and the channel refers to the checksum replica identification channel, then a warning should be issued.  Remodel! || Minor || Postpone ||
|| 445 || bamon should not be able to be 'null'. Remodel! || Minor || Postpone ||
|| 461-462 || cr should be able to be 'null'. Remodel! || Minor || Postpone ||
|| 473 || Remove this commented comment || Cosmetic || OK ||
|| 485 || Make ArgumentNotValid check on 'channelName'. And describe in Java-doc. || Cosmetic || OK ||
|| 496-497 || Change this error message: "The current channel name, '" + channelName + "' does not refer to an identification channel". Also log the error before issuing it. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileListJob.java', revision 937 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 121 || What's the purpose of this method? I think it confuses more than it helps, especially when it takes the outputFile as input, or change to be non-static and use field || Cosmetic || Postpone ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/distribute/ReplicaClientFactory.java', revision 1006 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 45-48 || Please remove _ in methodnames Also, this seems to emphsize the problem about needing a datastructure for this. || Cosmetic || Postpone ||
|| 57 || This needs to be changed, since the channels are not to contain nulls. Use instead the ReplicaType to determine the type of replica. The list of ReplicaIds can be extracted by using the function Channels.getUsedReplicaIds, and through the ReplicaIds the Replica can be found and thus the ReplicaType can be extracted.  A faster way to determine the replica type is to test whether ALL_BA, ANY_BA or THE_BAMON refers to the same channel, which is only the case for the ChecksumReplica. Though this way is a ugly hack.  Remodel! || Minor || Postpone ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/ArchiveSettings.java', revision 940 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 143-150 || Java-doc! || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/distribute/ReplicaClient.java', revision 959 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 53 || java-doc || Cosmetic || OK ||
|| 140 || Doc || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileBasedActiveBitPreservation.java', revision 939 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 40 || I'm a little scared that this import is added - it seems some separation of responsibility has been broken. Ah. It is unused || Cosmetic || OK ||
|| 295, 304 || A replica should generally not be called 'ba'. Though it might be all right in this function, just describe why. || Cosmetic || OK ||
|| 303 || Change the name to indicate that the checksum are retrieved from a Bitarchive and not a checksum archive. || Cosmetic || OK ||
|| 346-376 || Indentation || Cosmetic || OK ||
|| 388, 394 || Does this only handle bitarchives? change to replica or rep. || Cosmetic || OK ||
|| 502 || Check whether Bitarchive. Don't assume anything. Change to enum switch || Cosmetic ||Postpone ||
|| 554 || Again refering to one replica type only. Change to replicaChecksumSet. || Cosmetic || OK ||
|| 653 || Check whether Bitarchive. Don't assume anything. Change to enum switch || Cosmetic || Postpone ||
|| 815, 821 || Bitarchive? A checksum archive can also have damaged entries. Change to damagedReplica. || Cosmetic || OK ||
|| 831 || It needs to be ensured that the reference replica is a Bitarchive and not a checksum archive. Otherwise it can cause ugly problems. Change to enum switch || Cosmetic || Postpone ||
|| 874, 881 || again: damagedReplica. || Cosmetic || OK ||
|| 912, 916 || ba means BitArchive. This should also handle Checksum archives. Change to 'rep' or 'replica'. || Cosmetic || OK ||
|| 957, 961 || Describe why this function only handles bitarchives. (cannot get a file from a checksum archive) || Cosmetic || OK ||
|| 981-984 || Why is there a function which has not been implemented? || Minor || Postpone ||
|| 993-996 || Why is there another function which has not been implemented? || Minor || Postpone ||
|| 1005-1008 || Why has this function also not been implemented? || Minor || Postpone ||
|| 1023 || Do not use '.equals("")', change to '.isEmpty()'. || Cosmetic || OK ||
|| 1031 || 'l' is a bad name for a replica. change to rep. || Cosmetic || OK ||
|| 1040 || Handle this todo! || Cosmetic || Postpone ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/ArcRepositoryServer.java', revision 962 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 176-177 || The ReplicaClient should not be named 'bc', since it stands for BitarchiveClient. It should be remaned to 'rc'. || Cosmetic || OK ||
|| 199 || The ReplicaClient should not be named 'bc', since it stands for BitarchiveClient. It should be remaned to 'rc'. || Cosmetic || OK ||
|| 221-222 || The ReplicaClient should not be named 'bc', since it stands for BitarchiveClient. It should be remaned to 'rc'. || Cosmetic || OK ||
|| 234, 255 || Would it be a good idea to turn this into a batch job in the case of it not being a checksum replica? What happens now if I use this method on a bitarchive replica? There seems to be no logic for handling it? || Minor || Postpone ||
|| 243 || The ReplicaClient should be rename 'rc', since 'cc' refers to a ChecksumClient. || Cosmetic || OK ||
|| 253-257 || The message 'msg' is not mentioned in the java-doc || Cosmetic || OK ||
|| 261 || The ReplicaClient should be rename 'rc', since 'cc' refers to a ChecksumClient. || Cosmetic || OK ||
|| 278 || What is the '@Override' policy? Do we use override or do we not.  Send mail on mailing list. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/GetChecksumMessage.java', revision 1006 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 38 || Of course this should be sent to a specific replica! This means that the message should have the replica id as a variable and a constructor argument. || Minor || Postpone ||
|| 41-42 || Are we using serialVersionUID? If not, then this should be deleted. || Cosmetic || OK ||
|| 81-82 || HANDLE! Document that a null means that the file was not found. || Cosmetic || OK ||
|| 92 || ArgumentNotValid check || Cosmetic || OK ||
|| 111 || Add checksum and replica id || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/ChecksumArchiveAPI.java', revision 937 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 31-39 || Add a @see dk.netarkivet.archive.checksum.FileChecksumArchive, and eventually also the DatabaseChecksumArchive, when it has been implemented. || Cosmetic || OK ||
|| 40 || Shouldn't this have a common superclass with an API for BitArchives? Also, it is often considered bad practice to use words like "API" and "Interface" in interface names || Cosmetic || OK ||
|| 56 || Why would I need the checksum for that? I can understand that you need the wrong checksum, to make sure you only remove what you expect Make method "correct" not "remove" || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/CorrectMessage.java', revision 944 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 10 || This message should correct a bad entry in the archive, whether bad line in ChecksumArchive or a bad file in BitArchive. Not just 'Something'! || Minor || OK ||
|| 14-19 || Need java-doc. || Cosmetic || OK ||
|| 36-43 || Shouldn't this function be replaced by the same kind of 'getData()' function as in GetAllChecksumMessage or GetAllFilenamesMessage? || Cosmetic || OK ||
|| 54-62 || Doesn't really make sense on impl. || Cosmetic || OK ||
=== Comments on file 'trunk/scripts/sql/createBitpreservationDB.sql', revision 941 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
=== Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/Replica.java', revision 1006 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 247 || This should not be part of the 'return' statement in the java-doc. || Cosmetic || OK ||
|| 249 || Rename of function. It should be called getIdentificationChannel. The current name could easily be understood as the id of the channel, e.g. the channel-name. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/ArcRepository.java', revision 1006 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || There is a lot of references to BitArchiveStoreState. This class should be renamed to ReplicaStoreState, since it also deals with ChecksumArchives. || Cosmetic || OK ||
|| 74 || Describe that batch jobs are only executed on bitarchive replicas. || Cosmetic || OK ||
|| 169-173 || Again a strong indication that we really need a datastructure here. || Minor || Postpone ||
|| 182-184 || Make check for checksum replica, and if that is not the case, then throw an exception.  Actually, use a switch on the enum, so we get compile time checking of all cases handled. || Minor || Postpone ||
|| 222, 225-227, 229-245 || These methods seem unnecessary, since they only have one statement, that has a rather obvious effect || Cosmetic || OK ||
|| 248-253, 255-257 || Make new javadoc || Minor || OK ||
|| 260 || throws also ArgumentNotValid || Cosmetic || OK ||
|| 265 || This should not be a warning! Change back to 'log.info'. || Cosmetic || OK ||
|| 346-379 || REMODEL! || Minor || Postpone ||
|| 362, 368, 371 || Use enum switch || Minor || Postpone ||
|| 436 || NOT BITARCHIVES!!! It is called replicas when it also handle the checksum replicas. || Cosmetic || OK ||
|| 465, 470, 472 || This handles both kinds of replica, and should therefore not only refer to Bitarchives. || Cosmetic || OK ||
|| 482 || Stranger things happen at sea :) I vote we just remove the TODO, returning true is really the only sensible thing we can do || Cosmetic || OK ||
|| 492, 497, 499 || Why does this only refer to BitArchive replicas, when the function also handles Checksum replica? || Cosmetic || OK ||
|| 519, 522 || REPLICA CLIENT!!! it handles both kinds of replicas now! || Cosmetic || OK ||
|| 534-536 || Why doesn't Replica.getReplicaFromId throw UnknownID rather than return null? Then we could kill this method. || Cosmetic || Reject ||
|| 542-559 || Perhaps this method should be moved to Channels? I don't really like that we parse the channel names. || Cosmetic || Postpone ||
|| 583-586 || Change to HTML-tags. The current numeration looks bad in java-doc. || Cosmetic || OK ||
|| 602 || Describe why this function is called. || Cosmetic || OK ||
|| 719 || Consider: Is there any way we can avoid talking about messages in the ArcRepository class, and move that abstraction to the ArcRepositoryServer class? In that case, we could probably handle responses from batch jobs and checksum replies in the same message, rather than individually for checksum- and bitreplicas || Cosmetic || Postpone ||
|| 728-730 || Incorrect indentation. || Cosmetic || OK ||
|| 842 || Where does this end up? Will it be a reply with store failed? It certainly should be! || Minor || Postpone ||
|| 896-897 || The String is not called replicaId, but replicaChannelName! || Cosmetic || OK ||
|| 982, 987 || It is not the replica id but the identification channel of the replica. || Minor || OK ||
|| 1073 || The method in the message should be changed to getReplicaId(). || Cosmetic || OK ||
=== Comments on file 'trunk/webpages/BitPreservation/Bitpreservation-filestatus-checksum.jsp', revision 975 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||