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