8522
Comment:
|
← Revision 7 as of 2010-08-16 10:24:44 ⇥
8333
converted to 1.6 markup
|
Deletions are marked like this. | Additions are marked like this. |
Line 10: | Line 10: |
{{{ | {{{ |
Line 15: | Line 16: |
Line 18: | Line 18: |
|| Use different termilogy: wrong/bad => corrupt || Cosmetic || NOTOK || | || Use different termilogy: wrong/bad => corrupt || Cosmetic || OK || |
Line 23: | Line 23: |
|| 263 || Should we validate the badFile before sending it back? || Cosmetic || NOTOK || | || 263 || Should we validate the badFile before sending it back? || Cosmetic || REJECT || |
Line 26: | Line 26: |
|| 684 || Replace 'wrong' with 'corrupt' || Cosmetic || NOTOK || || 717, 721 || This should be 'removedFile' instead of retrieving the variable again. || Cosmetic || NOTOK || |
|| 684 || Replace 'wrong' with 'corrupt' || Cosmetic || OK || || 717, 721 || This should be 'removedFile' instead of retrieving the variable again. || Cosmetic || OK || |
Line 30: | Line 30: |
|| 79 || "String checksum" => "String badChecksum" || Cosmetic || NOTOK || || 173-176 || Issue warning, and return null. || Minor || NOTOK || |
|| 79 || "String checksum" => "String badChecksum" || Cosmetic || OK || || 173-176 || Issue warning, and return null. || Minor || OK || |
Line 34: | Line 34: |
|| 723-724 || Language: Change to "Make the file containing the corrupt entry be returned in the CorrectMessage || Cosmetic || NOTOK || | || 723-724 || Language: Change to "Make the file containing the corrupt entry be returned in the CorrectMessage || Cosmetic || OK || |
Line 37: | Line 37: |
|| 85 || Remove. This will not be implemented. || Cosmetic || NOTOK || || 138-139 || Split up into multiple commands || Cosmetic || NOTOK || || 159-160 || Split up into multiple commands || Cosmetic || NOTOK || || 446 || Describe, when this will be implemented || Cosmetic || NOTOK || || 469 || Describe, when this will be implemented || Cosmetic || NOTOK || || 515, 519 || Does not throw an UnknownID (or IOFailure? - perhaps indirectly) || Cosmetic || NOTOK || || 601 || Add when this will be implemented || Cosmetic || NOTOK || || 641 || typo: teh => the || Cosmetic || NOTOK || |
|| 85 || Remove. This will not be implemented. || Cosmetic || OK || || 138-139 || Split up into multiple commands || Cosmetic || OK || || 159-160 || Split up into multiple commands || Cosmetic || OK || || 446 || Describe, when this will be implemented || Cosmetic || OK || || 469 || Describe, when this will be implemented || Cosmetic || OK || || 515, 519 || Does not throw an UnknownID (or IOFailure? - perhaps indirectly) || Cosmetic || OK || || 601 || Add when this will be implemented || Cosmetic || OK || || 641 || typo: teh => the || Cosmetic || OK || |
Line 47: | Line 47: |
|| 358-359 || Change to "Checksum job message submitted for file '" + filename + "' with message id '" + msg.getID + "' || Cosmetic || NOTOK || | || 358-359 || Change to "Checksum job message submitted for file '" + filename + "' with message id '" + msg.getID + "' || Cosmetic || OK || |
Line 50: | Line 50: |
|| 72 || Does "incorrectChecksum" refer to an undefined variable? || Cosmetic || NOTOK || | || 72 || Does "incorrectChecksum" refer to an undefined variable? || Cosmetic || OK || |
Line 53: | Line 53: |
|| 60-61 || What prevents you from removing this constructor, now? || Cosmetic || NOTOK || || 98 || Typo: "fopr .." => "for the file" || Cosmetic || NOTOK || || 101-102 || Add missing argument-check || Cosmetic || NOTOK || |
|| 60-61 || What prevents you from removing this constructor, now? || Cosmetic || OK || || 98 || Typo: "fopr .." => "for the file" || Cosmetic || OK || || 101-102 || Add missing argument-check || Cosmetic || OK || |
Line 58: | Line 58: |
|| 66 || change "theCR" to "theChecksumChannel" or "theChecksumQueue" || Cosmetic || NOTOK || || 192 || What is this TODO about? I don't understand it || Cosmetic || NOTOK || || 192 || Remove! The todo is fixed || Cosmetic || NOTOK || |
|| 66 || change "theCR" to "theChecksumChannel" or "theChecksumQueue" || Cosmetic || OK || || 192 || What is this TODO about? I don't understand it || Cosmetic || OK || || 192 || Remove! The todo is fixed || Cosmetic || OK || |
Line 63: | Line 63: |
|| 107, 110, 116 || "First" => "In the first stage" "Second" => In the second stage" "Third" => "In the third stage" || Cosmetic || NOTOK || || 112 || an UploadMessage || Cosmetic || NOTOK || || 240-251 || This description here is redundant || Cosmetic || NOTOK || || 258 || Typo: Recieving => Receiving || Cosmetic || NOTOK || || 258, 298-299 || Receiving || Cosmetic || NOTOK || || 280-281 || Redundant description || Cosmetic || NOTOK || || 289-291 || Redundant description || Cosmetic || NOTOK || || 308 || Typo: filed => failed || Cosmetic || NOTOK || || 333-340 || redundant description || Cosmetic || NOTOK || || 368, 371 || Finish the two sentences. || NA || NOTOK || || 373 || argument validation || Cosmetic || NOTOK || || 389 || argument validation + javadoc || Cosmetic || NOTOK || || 405 || argument validation + javadoc. || Cosmetic || NOTOK || || 444 || Trouble while handling batch request => Unable to handle batch request due to unexpected exception || Cosmetic || NOTOK || || 561 || Which other exceptions are foreseen here besides the IOFailure thrown in case of unhandled message type? The called method handle their own exceptions, so none. The 'try - catch' is redundant if the reply is put into the last 'else' clause. || Cosmetic || NOTOK || || 640 || Change to: Fetch the contents of the batchresultfile || Cosmetic || NOTOK || || 657 || Include the others in the log entry || Cosmetic || NOTOK || || 664 || explain what valid means here || Cosmetic || NOTOK || |
|| 107, 110, 116 || "First" => "In the first stage" "Second" => In the second stage" "Third" => "In the third stage" || Cosmetic || OK || || 112 || an UploadMessage || Cosmetic || OK || || 240-251 || This description here is redundant || Cosmetic || OK || || 258 || Typo: Recieving => Receiving || Cosmetic || OK || || 258, 298-299 || Receiving || Cosmetic || OK || || 280-281 || Redundant description || Cosmetic || OK || || 289-291 || Redundant description || Cosmetic || OK || || 308 || Typo: filed => failed || Cosmetic || OK || || 333-340 || redundant description || Cosmetic || OK || || 368, 371 || Finish the two sentences. || NA || OK || || 373 || argument validation || Cosmetic || OK || || 389 || argument validation + javadoc || Cosmetic || OK || || 405 || argument validation + javadoc. || Cosmetic || OK || || 444 || Trouble while handling batch request => Unable to handle batch request due to unexpected exception || Cosmetic || OK || || 561 || Which other exceptions are foreseen here besides the IOFailure thrown in case of unhandled message type? The called method handle their own exceptions, so none. The 'try - catch' is redundant if the reply is put into the last 'else' clause. || Cosmetic || OK || || 640 || Change to: Fetch the contents of the batchresultfile || Cosmetic || OK || || 657 || Include the others in the log entry || Cosmetic || OK || || 664 || explain what valid means here || Cosmetic || OK || |
Line 83: | Line 83: |
|| 296 || This method is redundant: the method only forwards to the getChecksum method || Cosmetic || NOTOK || || 347 || Wrong spelling of 'successful' || Cosmetic || NOTOK || || 363 || Wrong checkNotNull message || Cosmetic || NOTOK || || 402 || Replace log message with "Finding missing files in directory '" || Cosmetic || NOTOK || || 413 || Add to comment what difference set 1 is || Cosmetic || NOTOK || || 431 || Add to comment what difference set 2 is || Cosmetic || NOTOK || || 449 || Replace log message with "Finished finding missing files" || Cosmetic || NOTOK || || 472-474 || Make an external variable for value of "ArcRepositoryClientFactory.getPreservationInstance().getAllFilenames(replica.getId))" so this value can be checked before calling the copyFile method || Cosmetic || NOTOK || || 478 || Don't use the word 'wrong'. Use the word 'corrupt' instead. || Cosmetic || NOTOK || || 619-620 || Make an external variable for value of "ArcRepositoryClientFactory.getPreservationInstance().getAllChecksums(replica.getId))" so this value can be checked before calling the copyFile method || Cosmetic || NOTOK || || 718-720 || Please change to: Check that the files we want to restore are indeed missing on the replica on which we are restoring them, and presnt in admin data and the reference replica. If so, upload the missing files from the reference replica to this replica. || Cosmetic || NOTOK || || 873 || Add that: We don't wait for an acknowledgement that admin data has indeed been updated. || Cosmetic || NOTOK || || 916 || change "a bad entry" to "an entry" || Cosmetic || NOTOK || || 962 || When will this be implemented? || Cosmetic || NOTOK || || 975 || When will this be implemented? || Cosmetic || NOTOK || || 988 || Change to "String... filenames" || Cosmetic || NOTOK || || 990 || When will this be implemented? || Cosmetic || NOTOK || || 1023 || When will this be implemented? || Cosmetic || NOTOK || |
|| 296 || This method is redundant: the method only forwards to the getChecksum method || Cosmetic || OK || || 347 || Wrong spelling of 'successful' || Cosmetic || OK || || 363 || Wrong checkNotNull message || Cosmetic || OK || || 402 || Replace log message with "Finding missing files in directory '" || Cosmetic || OK || || 413 || Add to comment what difference set 1 is || Cosmetic || OK || || 431 || Add to comment what difference set 2 is || Cosmetic || OK || || 449 || Replace log message with "Finished finding missing files" || Cosmetic || OK || || 472-474 || Make an external variable for value of "ArcRepositoryClientFactory.getPreservationInstance().getAllFilenames(replica.getId))" so this value can be checked before calling the copyFile method || Cosmetic || OK || || 478 || Don't use the word 'wrong'. Use the word 'corrupt' instead. || Cosmetic || OK || || 619-620 || Make an external variable for value of "ArcRepositoryClientFactory.getPreservationInstance().getAllChecksums(replica.getId))" so this value can be checked before calling the copyFile method || Cosmetic || OK || || 718-720 || Please change to: Check that the files we want to restore are indeed missing on the replica on which we are restoring them, and presnt in admin data and the reference replica. If so, upload the missing files from the reference replica to this replica. || Cosmetic || OK || || 873 || Add that: We don't wait for an acknowledgement that admin data has indeed been updated. || Cosmetic || OK || || 916 || change "a bad entry" to "an entry" || Cosmetic || OK || || 962 || When will this be implemented? || Cosmetic || OK || || 975 || When will this be implemented? || Cosmetic || OK || || 988 || Change to "String... filenames" || Cosmetic || OK || || 990 || When will this be implemented? || Cosmetic || OK || || 1023 || When will this be implemented? || Cosmetic || OK || |
Line 103: | Line 103: |
|| 229-230 || Remove this @throws clause || Cosmetic || NOTOK || || 243 || why was this line added? || Cosmetic || NOTOK || || 247 || Typo: be send => be sent || Cosmetic || NOTOK || || 286-288 || Document that a Argument not valid is thrown || Cosmetic || NOTOK || || 313 || remove || Cosmetic || NOTOK || |
|| 229-230 || Remove this @throws clause || Cosmetic || OK || || 243 || why was this line added? || Cosmetic || OK || || 247 || Typo: be send => be sent || Cosmetic || OK || || 286-288 || Document that a Argument not valid is thrown || Cosmetic || OK || || 313 || remove || Cosmetic || OK || |
Review (NS-145): Assignment B.2.2b: Make bitarchive replica use new messages.
Author |
Jonas |
Moderator |
Jonas |
State |
Review |
Objectives
See FR 1823.
Total Time Used (Coding,Documentation,Review):
Time use (Coding,Documentation,Review) SVC: 1.0 JOLF: 5.0
General comments:
Description |
Classification |
Status |
Use different termilogy: wrong/bad => corrupt |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumFileServer.java', revision 1246
Lines |
Description |
Classification |
Status |
263 |
Should we validate the badFile before sending it back? |
Cosmetic |
REJECT |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/JMSArcRepositoryClient.java', revision 1246
Lines |
Description |
Classification |
Status |
684 |
Replace 'wrong' with 'corrupt' |
Cosmetic |
OK |
717, 721 |
This should be 'removedFile' instead of retrieving the variable again. |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/CorrectMessage.java', revision 1246
Lines |
Description |
Classification |
Status |
79 |
"String checksum" => "String badChecksum" |
Cosmetic |
OK |
173-176 |
Issue warning, and return null. |
Minor |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/checksum/FileChecksumArchive.java', revision 1246
Lines |
Description |
Classification |
Status |
723-724 |
Language: Change to "Make the file containing the corrupt entry be returned in the CorrectMessage |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DatabaseBasedActiveBitPreservation.java', revision 1254
Lines |
Description |
Classification |
Status |
85 |
Remove. This will not be implemented. |
Cosmetic |
OK |
138-139 |
Split up into multiple commands |
Cosmetic |
OK |
159-160 |
Split up into multiple commands |
Cosmetic |
OK |
446 |
Describe, when this will be implemented |
Cosmetic |
OK |
469 |
Describe, when this will be implemented |
Cosmetic |
OK |
515, 519 |
Does not throw an UnknownID (or IOFailure? - perhaps indirectly) |
Cosmetic |
OK |
601 |
Add when this will be implemented |
Cosmetic |
OK |
641 |
typo: teh => the |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/ArcRepository.java', revision 1242
Lines |
Description |
Classification |
Status |
358-359 |
Change to "Checksum job message submitted for file '" + filename + "' with message id '" + msg.getID + "' |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/checksum/ChecksumArchive.java', revision 1246
Lines |
Description |
Classification |
Status |
72 |
Does "incorrectChecksum" refer to an undefined variable? |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/RemoveAndGetFileMessage.java', revision 1246
Lines |
Description |
Classification |
Status |
60-61 |
What prevents you from removing this constructor, now? |
Cosmetic |
OK |
98 |
Typo: "fopr .." => "for the file" |
Cosmetic |
OK |
101-102 |
Add missing argument-check |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumClient.java', revision 1247
Lines |
Description |
Classification |
Status |
66 |
change "theCR" to "theChecksumChannel" or "theChecksumQueue" |
Cosmetic |
OK |
192 |
What is this TODO about? I don't understand it |
Cosmetic |
OK |
192 |
Remove! The todo is fixed |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveMonitorServer.java', revision 1251
Lines |
Description |
Classification |
Status |
107, 110, 116 |
"First" => "In the first stage" "Second" => In the second stage" "Third" => "In the third stage" |
Cosmetic |
OK |
112 |
Cosmetic |
OK |
|
240-251 |
This description here is redundant |
Cosmetic |
OK |
258 |
Typo: Recieving => Receiving |
Cosmetic |
OK |
258, 298-299 |
Receiving |
Cosmetic |
OK |
280-281 |
Redundant description |
Cosmetic |
OK |
289-291 |
Redundant description |
Cosmetic |
OK |
308 |
Typo: filed => failed |
Cosmetic |
OK |
333-340 |
redundant description |
Cosmetic |
OK |
368, 371 |
Finish the two sentences. |
NA |
OK |
373 |
argument validation |
Cosmetic |
OK |
389 |
argument validation + javadoc |
Cosmetic |
OK |
405 |
argument validation + javadoc. |
Cosmetic |
OK |
444 |
Trouble while handling batch request => Unable to handle batch request due to unexpected exception |
Cosmetic |
OK |
561 |
Which other exceptions are foreseen here besides the IOFailure thrown in case of unhandled message type? The called method handle their own exceptions, so none. The 'try - catch' is redundant if the reply is put into the last 'else' clause. |
Cosmetic |
OK |
640 |
Change to: Fetch the contents of the batchresultfile |
Cosmetic |
OK |
657 |
Include the others in the log entry |
Cosmetic |
OK |
664 |
explain what valid means here |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileBasedActiveBitPreservation.java', revision 1254
Lines |
Description |
Classification |
Status |
296 |
This method is redundant: the method only forwards to the getChecksum method |
Cosmetic |
OK |
347 |
Wrong spelling of 'successful' |
Cosmetic |
OK |
363 |
Wrong checkNotNull message |
Cosmetic |
OK |
402 |
Replace log message with "Finding missing files in directory '" |
Cosmetic |
OK |
413 |
Add to comment what difference set 1 is |
Cosmetic |
OK |
431 |
Add to comment what difference set 2 is |
Cosmetic |
OK |
449 |
Replace log message with "Finished finding missing files" |
Cosmetic |
OK |
472-474 |
Make an external variable for value of "ArcRepositoryClientFactory.getPreservationInstance().getAllFilenames(replica.getId))" so this value can be checked before calling the copyFile method |
Cosmetic |
OK |
478 |
Don't use the word 'wrong'. Use the word 'corrupt' instead. |
Cosmetic |
OK |
619-620 |
Make an external variable for value of "ArcRepositoryClientFactory.getPreservationInstance().getAllChecksums(replica.getId))" so this value can be checked before calling the copyFile method |
Cosmetic |
OK |
718-720 |
Please change to: Check that the files we want to restore are indeed missing on the replica on which we are restoring them, and presnt in admin data and the reference replica. If so, upload the missing files from the reference replica to this replica. |
Cosmetic |
OK |
873 |
Add that: We don't wait for an acknowledgement that admin data has indeed been updated. |
Cosmetic |
OK |
916 |
change "a bad entry" to "an entry" |
Cosmetic |
OK |
962 |
When will this be implemented? |
Cosmetic |
OK |
975 |
When will this be implemented? |
Cosmetic |
OK |
988 |
Change to "String... filenames" |
Cosmetic |
OK |
990 |
When will this be implemented? |
Cosmetic |
OK |
1023 |
When will this be implemented? |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveClient.java', revision 1247
Lines |
Description |
Classification |
Status |
229-230 |
Remove this @throws clause |
Cosmetic |
OK |
243 |
why was this line added? |
Cosmetic |
OK |
247 |
Typo: be send => be sent |
Cosmetic |
OK |
286-288 |
Document that a Argument not valid is thrown |
Cosmetic |
OK |
313 |
remove |
Cosmetic |
OK |