Differences between revisions 1 and 7 (spanning 6 versions)
⇤ ← Revision 1 as of 2009-10-26 08:07:01 →
Size: 9952
Editor: JonasFrellesen
Comment:
← Revision 7 as of 2010-08-16 10:25:15 → ⇥
Size: 9786
Editor: localhost
Comment: converted to 1.6 markup
Deletions are marked like this. Additions are marked like this.
Line 9: Line 9:
Line 11: Line 10:
Line 14: Line 12:
{{{ 
{{{
Line 19: Line 18:
Line 26: Line 24:
|| 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 ||		 || 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 ||
Line 31: Line 29:
|| 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 ||		 || 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 ||
Line 35: Line 33:
|| 410 || Is this really the 'replicaId' and not the 'replica identification channel name'? || Cosmetic || NOTOK || || 410 || Is this really the 'replicaId' and not the 'replica identification channel name'? || Cosmetic || OK ||
Line 38: Line 36:
|| 176-177 || This does not concern the value, only the path to the value within the settings. || Cosmetic || NOTOK || || 176-177 || This does not concern the value, only the path to the value within the settings. || Cosmetic || OK ||
Line 41: Line 39:
|| 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 ||		 || 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 ||
Line 45: Line 43:
|| 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 ||		 || 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 ||
Line 60: Line 58:
|| 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 ||		 || 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 ||
Line 65: Line 63:
|| 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 ||		 || 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 ||
Line 69: Line 67:
|| 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 ||		 || 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 ||
Line 74: Line 72:
|| 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 ||		 || 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 ||
Line 78: Line 76:
|| 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 ||		 || 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 ||
Line 92: Line 90:
|| 173-174 || This should be replaced by 'Replica.getKnown()'. || Cosmetic || NOTOK || || 173-174 || This should be replaced by 'Replica.getKnown()'. || Cosmetic || OK ||
Line 95: Line 93:
|| 73 || Use file instead of RemoteFile. || Cosmetic || NOTOK || || 73 || Use file instead of RemoteFile. || Cosmetic || OK ||
Line 98: Line 96:
|| 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 ||		 || 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 ||

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

IssuesFromNs108 (last edited 2010-08-16 10:25:15 by localhost)