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	NOTOK
215	[spelling]: receving => receiving	Cosmetic	NOTOK
253-254	Describe that this exception can be thrown.	Cosmetic	NOTOK

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	NOTOK
105, 133-134	Describe that a 'UnknownID' exception can be thrown.	Cosmetic	NOTOK

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	NOTOK

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	NOTOK

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	NOTOK
144	Is this correct. Isn't this the new checksum? Better variable naming required.	Cosmetic	NOTOK

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	NOTOK
296	It should now handled checksum replicas! There are at least a method which should do it.	Cosmetic	NOTOK
307-313	Describe what is done here!	Cosmetic	NOTOK
312	It should say 'unhandled replica type' instead.	Cosmetic	NOTOK
318-320	This java-doc should be removed. It is implemented now!	Cosmetic	NOTOK
322-323	implemented the intended method (using the word correct can be confusing in this case).	Cosmetic	NOTOK
345-346	Is this necessary? Two entries for the same file should be treated the same way as for the bitarchive replica.	Cosmetic	NOTOK
348-350	Describe what is done here	Cosmetic	NOTOK
355	handle or describe why not. In any case log the error.	Minor	NOTOK
362	Not just any batchjob. It sends a ChecksumJob which is limitted to some specific files.	Cosmetic	NOTOK
518	not replcia! it is called replica	Cosmetic	NOTOK
727-728	IS there a corresponding log: Bit integrity check completed ?	Cosmetic	NOTOK
734	It is not described in the method definition or java-doc that this exception can be thrown.	Cosmetic	NOTOK

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	NOTOK
218, 224, 231, 244	Instead of "TODO: implemented.." write "Will be implemented in release 3.12.0"	Cosmetic	NOTOK
259	Remove line	Cosmetic	NOTOK

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	NOTOK
58	Does the super method validate to and replyTo? In that case the validation of those fields are not needed here.	Cosmetic	NOTOK

Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/GetAllChecksumsMessage.java', revision 1078

Lines	Description	Classification	Status
46	Bad variable naming	Cosmetic	NOTOK
71	Shouldn't this method be named setResultingFile	Cosmetic	NOTOK
78	Add to javadoc: This method can only be called once, because it deletes the remoteFile, and sets the remoteFile field to null.	Cosmetic	NOTOK

Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/CorrectMessage.java', revision 1054

Lines	Description	Classification	Status
26	Remove this unused import.	Cosmetic	NOTOK
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	NOTOK

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	NOTOK
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	NOTOK
241	Describe why this is called if the file already exists.	Cosmetic	NOTOK
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	NOTOK
281-285	Reformulate javadoc. What do you mention "the correct archive file". The name of the method is saveArchiveFile. Maybe rename method	Cosmetic	NOTOK
451	describe why this has to be synchronized around the checksum file.	Cosmetic	NOTOK
506	calculate the checksum from the file instead.	Cosmetic	NOTOK
547	it does not throw IOFailure any more	Cosmetic	NOTOK
599	It should not be a RemoteFile but instead just a file.	Cosmetic	NOTOK
608-610	ARGUMENT NOT VALID?	Cosmetic	NOTOK
632-633	Use the method in ChecksumJob for making a checksum entry string ('makeLine').	Cosmetic	NOTOK
636	as above	Cosmetic	NOTOK

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	NOTOK

Comments on file 'trunk/src/dk/netarkivet/archive/checksum/ChecksumArchive.java', revision 1054

Lines	Description	Classification	Status
73	Use file instead of RemoteFile.	Cosmetic	NOTOK

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	NOTOK
199	Remove initial Linebreak: "\n" in logmessage	Cosmetic	NOTOK
210	This doesn't make any sense for me	Cosmetic	NOTOK
218-219	Remove linebreak after "rf"	Cosmetic	NOTOK
245	Missing argument validation	Cosmetic	NOTOK
265	Missing argument validation	Cosmetic	NOTOK
281	Missing argument validation	Cosmetic	NOTOK
298	Missing argument validation	Cosmetic	NOTOK
315	Missing argument Validation	Cosmetic	NOTOK