⇤ ← Revision 1 as of 2009-03-24 16:52:29
17655
Comment:
|
17632
|
Deletions are marked like this. | Additions are marked like this. |
Line 37: | Line 37: |
Line 39: | Line 41: |
|| 125 || ArgumentNotValid.checkNotNullOrEmpty(appID); ? || Cosmetic || NOTOK || || 131 || "from" => "from bitarchive" || Cosmetic || NOTOK || || 221-229 || Does not check ArgumentNotValid on: 'noOfFilesProcessed', 'filesFailed', 'remoteFile', 'errMsg', 'exceptions'. || Cosmetic || NOTOK || || 305, 314 || Perhaps consider to change to 'private' with 'get' and 'set' methods instead of public local variables. || Cosmetic || NOTOK || || 314 || Error messages is appended by 'String' + 'String'. Perhaps change to StringBuilder? || Cosmetic || NOTOK || || 354-355 || Log this || Cosmetic || NOTOK || || 370 || Check for ArgumentNotValid? || Cosmetic || NOTOK || || 412-421 || 'found' only used once. Thus change to if(!missingRespondents.remove(bitarchiveID)) { ... } OR write comment about 'found' || Cosmetic || NOTOK || || 522 || check for ArgumentNotValid? || Cosmetic || NOTOK || |
|| 125 || ArgumentNotValid.checkNotNullOrEmpty(appID); ? || Cosmetic ||OK || || 131 || "from" => "from bitarchive" || Cosmetic || OK || || 221-229 || Does not check ArgumentNotValid on: 'noOfFilesProcessed', 'filesFailed', 'remoteFile', 'errMsg', 'exceptions'. || Cosmetic || OK || || 305, 314 || Perhaps consider to change to 'private' with 'get' and 'set' methods instead of public local variables. || Cosmetic ||POSTPONED || || 314 || Error messages is appended by 'String' + 'String'. Perhaps change to StringBuilder? || Cosmetic ||POSTPONED|| || 354-355 || Log this || Cosmetic || OK || || 370 || Check for ArgumentNotValid? || Cosmetic ||REJECTED || || 412-421 || 'found' only used once. Thus change to if(!missingRespondents.remove(bitarchiveID)) { ... } OR write comment about 'found' || Cosmetic || OK || || 522 || check for ArgumentNotValid? || Cosmetic || OK || |
Line 66: | Line 68: |
|| 211-213 || Identical messages -> make into string before use. || Cosmetic || NOTOK || || 268-271 || ArgumentNotValid.checkTrue(file.isFile(), "...."); || Cosmetic || NOTOK || || 273 || Better to use StringBuilder. || Cosmetic || NOTOK || || 387, 407 || Examine the replyMsg, and check if it is ok. If it is not ok, log a message at level WARN || Cosmetic || NOTOK || || 412, 426 || Examine the replyMsg, and check if it is ok. If it is not ok, log a message at level WARN || Cosmetic || NOTOK || |
|| 211-213 || Identical messages -> make into string before use. || Cosmetic || OK || || 268-271 || ArgumentNotValid.checkTrue(file.isFile(), "...."); || Cosmetic || OK || || 273 || Better to use StringBuilder. || Cosmetic || OK || || 387, 407 || Examine the replyMsg, and check if it is ok. If it is not ok, log a message at level WARN || Cosmetic || OK || || 412, 426 || Examine the replyMsg, and check if it is ok. If it is not ok, log a message at level WARN || Cosmetic || OK || |
Review (NS-27): Review of changes to batch framework
Author |
Søren |
Moderator |
Søren |
State |
Closed |
Objectives
To review the current code in the NetarchiveSuite batch framework, including the tool RunBatch. The review includes a review of all the lines in these files: /trunk/src/dk/netarkivet/common/utils/batch/LoadableJarBatchJob.java /trunk/src/dk/netarkivet/common/utils/batch/FileBatchJob.java /trunk/src/dk/netarkivet/common/utils/batch/LoadableFileBatchJob.java /trunk/src/dk/netarkivet/common/utils/batch/BatchLocalFiles.java /trunk/src/dk/netarkivet/common/utils/batch/ByteClassLoader.java /trunk/src/dk/netarkivet/common/utils/batch/BatchFilter.java /trunk/src/dk/netarkivet/common/utils/arc/ARCBatchJob.java /trunk/src/dk/netarkivet/archive/tools/RunBatch.java /trunk/src/dk/netarkivet/common/distribute/arcrepository/BatchStatus.java trunk/src/dk/netarkivet/common/utils/ExtractCDX These files are only to be reviewed in the parts defined by the given linenumbers: trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveAdmin trunk/src/dk/netarkivet/archive/arcrepository/JMSArcRepositoryClient trunk/src/dk/netarkivet/common/distribute/arcrepository/TrivialArcRepositoryClient trunk/src/dk/netarkivet/common/distribute/arcrepository/LocalArcRepositoryClient trunk/src/dk/netarkivet/archive/bitarchive/Bitarchive trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveMonitor trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveMonitorServer trunk/src/dk/netarkivet/archive/bitarchive/distribute/BatchEndedMessage trunk/build.xml
Summary
Review with JOLF completed. Follow up by SVC
General comments:
Description |
Classification |
Status |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveMonitor.java', revision 544
Lines |
Description |
Classification |
Status |
125 |
ArgumentNotValid.checkNotNullOrEmpty(appID); ? |
Cosmetic |
OK |
131 |
"from" => "from bitarchive" |
Cosmetic |
OK |
221-229 |
Does not check ArgumentNotValid on: 'noOfFilesProcessed', 'filesFailed', 'remoteFile', 'errMsg', 'exceptions'. |
Cosmetic |
OK |
305, 314 |
Perhaps consider to change to 'private' with 'get' and 'set' methods instead of public local variables. |
Cosmetic |
POSTPONED |
314 |
Error messages is appended by 'String' + 'String'. Perhaps change to StringBuilder? |
Cosmetic |
POSTPONED |
354-355 |
Log this |
Cosmetic |
OK |
370 |
Check for ArgumentNotValid? |
Cosmetic |
REJECTED |
412-421 |
'found' only used once. Thus change to if(!missingRespondents.remove(bitarchiveID)) { ... } OR write comment about 'found' |
Cosmetic |
OK |
522 |
check for ArgumentNotValid? |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/tools/RunBatch.java', revision 737
Lines |
Description |
Classification |
Status |
149, 152 |
Add constants for ".class", and ".jar", and maybe "." |
Cosmetic |
NOTOK |
164 |
Spell CWD out: Current working directory |
Cosmetic |
NOTOK |
202, 204 |
public or private variables? |
Cosmetic |
NOTOK |
205-206 |
What is meant by this comment? Add TODO: Use the HelpFormatter class to print out Usage information |
Cosmetic |
NOTOK |
215, 217, 220, 224, 227, 234, 237 |
Consider replacing by constants |
Cosmetic |
NOTOK |
220-221 |
Indentation |
Cosmetic |
NOTOK |
222 |
Odd place for an empty line. |
Cosmetic |
NOTOK |
241 |
Javadoc. public/private/protected not defined for this method? |
Cosmetic |
NOTOK |
252 |
Javadoc. public/private/protected not defined for this method? |
Cosmetic |
NOTOK |
266-267 |
Two empty lines in a row. |
Cosmetic |
NOTOK |
268-269 |
Shouldn't variables be placed next to each other, and not randomly in the middle of the methods? |
Cosmetic |
NOTOK |
340 |
change "," to a constant. |
Cosmetic |
NOTOK |
355 |
Make into bug or feature request (if not already) and refer to this. Or implement |
Cosmetic |
NOTOK |
505-506 |
Two empty lines in a row |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/JMSArcRepositoryClient.java', revision 544
Lines |
Description |
Classification |
Status |
211-213 |
Identical messages -> make into string before use. |
Cosmetic |
OK |
268-271 |
ArgumentNotValid.checkTrue(file.isFile(), "...."); |
Cosmetic |
OK |
273 |
Better to use StringBuilder. |
Cosmetic |
OK |
387, 407 |
Examine the replyMsg, and check if it is ok. If it is not ok, log a message at level WARN |
Cosmetic |
OK |
412, 426 |
Examine the replyMsg, and check if it is ok. If it is not ok, log a message at level WARN |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/FileBatchJob.java', revision 548
Lines |
Description |
Classification |
Status |
51 |
".*" should perhaps be a constants (static final) variable. |
Cosmetic |
NOTOK |
109 |
".*" as a constant (static final) variable (same constant as previous) |
Cosmetic |
NOTOK |
138 |
Make an OR_SEPARATOR constant for "|" |
Cosmetic |
NOTOK |
164, 167 |
It does not need to be ARC-files we're processing |
Cosmetic |
NOTOK |
174-175, 177-178 |
It does not need to be ARC-files we're processing |
Cosmetic |
NOTOK |
206, 222 |
Log the case, if maxExceptions have been reached |
Cosmetic |
NOTOK |
235 |
Log the case, if maxExceptions have been reached |
Cosmetic |
NOTOK |
259 |
Add a TODO, that this should/could be a setting |
Cosmetic |
NOTOK |
330 |
Replace -1 with UNKNOWN_OFFSET |
Cosmetic |
NOTOK |
393-398 |
Javadoc this method. |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/BatchFilter.java', revision 548
Lines |
Description |
Classification |
Status |
General |
Rename BatchFilter as ARCBatchFilter. |
Cosmetic |
NOTOK |
117 |
Make comment about 'mimetypeIsOk' checks for ArgumentNotValid, or check the mimetype for ArgumentNotValid. |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/ByteClassLoader.java', revision 548
Lines |
Description |
Classification |
Status |
64 |
Add local variables for the 'null', and '0' constants: String name = null; int offset = 0; |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/BatchLocalFiles.java', revision 548
Lines |
Description |
Classification |
Status |
43 |
javadoc |
Cosmetic |
NOTOK |
49 |
Replace "used for batching" by "processed by the batch job" |
Cosmetic |
NOTOK |
55-58 |
This would be the same as: ArgumentNotValid.checkNotNull(incomingFile[i], "Null element at index " + i + " in file lise for batch."); Thus using the check function to check for NotNull. |
Cosmetic |
NOTOK |
84 |
Should we add the initialization exception to the list of exceptions for job? Add as TODO |
Cosmetic |
NOTOK |
90 |
Should we add this finish exception to the list of exceptions for job? Add as TODO |
Cosmetic |
NOTOK |
108-109 |
Should we add this exception thrown during processing to the list of exceptions for job? Add as TODO |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveMonitorServer.java', revision 626
Lines |
Description |
Classification |
Status |
48 |
language: "resent" i.e. forwarded |
Cosmetic |
NOTOK |
74 |
Rename con as jmscon |
Cosmetic |
NOTOK |
120 |
Consider checking that inbMsg is not null |
Cosmetic |
NOTOK |
144 |
Consider checking that beMsg is not null |
Cosmetic |
NOTOK |
176 |
Consider checking that hbMsg is not null |
Cosmetic |
NOTOK |
192 |
'o' is not a very good variable name og an observable object. Use 'obs' or equivilent; |
Cosmetic |
NOTOK |
202, 206, 212 |
Add respectively "an", "an", and "a" after "Received" |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BatchEndedMessage.java', revision 544
Lines |
Description |
Classification |
Status |
45 |
"to identifier for" => "to identify" |
Cosmetic |
NOTOK |
48 |
Rename "BA_ApplicationId" as "BitarchiveApplicationId" |
Cosmetic |
NOTOK |
59-61 |
three empty lines in a row. Delete two of them. |
Cosmetic |
NOTOK |
73 |
rename to BitarchiveApplicationId |
Cosmetic |
NOTOK |
76 |
"he remote file" => "The remote file" |
Cosmetic |
NOTOK |
177 |
Add ArgumentNotValid check |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/LoadableJarBatchJob.java', revision 760
Lines |
Description |
Classification |
Status |
116 |
Add a JAVA_PACKAGE_SEPARATOR constant for "." Add a DIRECTORY_SEPARATOR constant for "/" |
Cosmetic |
NOTOK |
119 |
Replace "No data loaded!!!!!" with "No data loaded for class with name '" + className + "'." |
Cosmetic |
NOTOK |
157 |
Check argument os |
Cosmetic |
NOTOK |
170 |
'Cannout' = spelling error. |
Cosmetic |
NOTOK |
192 |
"Finish up" => "Finish" |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/BatchStatus.java', revision 544
Lines |
Description |
Classification |
Status |
35 |
Delete line |
Cosmetic |
NOTOK |
58-59 |
".., that run this batchJob" => ".., that ran this batch job" |
Cosmetic |
NOTOK |
66-70 |
Check for ArgumentNotValid. |
Cosmetic |
NOTOK |
90 |
Make "ALL_BITARCHIVE_APPS" a constant |
Cosmetic |
NOTOK |
160 |
".. to copy into '" => ".. to copy into target file '" |
Cosmetic |
NOTOK |
161 |
"batch job on" => "batch job run on " |
Cosmetic |
NOTOK |
168 |
"so this" => so this method |
Cosmetic |
NOTOK |
187 |
the variable 'stream' cannot be printed out. Just write "append to output stream". |
Cosmetic |
NOTOK |
188 |
"batch job on" => batch job run on bitarchive application |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/common/utils/cdx/CDXUtils.java', revision 775
Lines |
Description |
Classification |
Status |
57 |
Check args arcfile and cdxstream |
Cosmetic |
NOTOK |
59 |
Remove superfluous whitespace |
Cosmetic |
NOTOK |
75 |
Remove superfluous whitespace |
Cosmetic |
NOTOK |
101-105 |
replace with: ArgumentNotValid.checkTrue(arcFileDirectory.isDirectory() && arcFileDirectory.canRead(), "..."); |
Cosmetic |
NOTOK |
106-110 |
replace with: ArgumentNotValid.checkTrue(cdxFileDirectory.isDirectory() && cdxFileDirectory.canRead(), "..."); |
Cosmetic |
NOTOK |
140 |
could be written if (!exceptions.isEmpty()) |
Cosmetic |
NOTOK |
Comments on file 'trunk/build.xml', revision 762
Lines |
Description |
Classification |
Status |
33-36, 71-73 |
Only three of the four new values are included. Add the fourth |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/Bitarchive.java', revision 774
Lines |
Description |
Classification |
Status |
106 |
ArgumentNotValid.checkNotNegative on 'index' ? |
Cosmetic |
NOTOK |
112 |
don't use 'arcfile' before it is checked for 'ArgumentNotValid' on the following line. |
Cosmetic |
NOTOK |
136-140 |
Remove uncommented code |
Cosmetic |
NOTOK |
183 |
check for ArgumentNotValid before use of argument-variables. |
Cosmetic |
NOTOK |
261-264 |
Identical messages, use a string. |
Cosmetic |
NOTOK |
318 |
Move log after argument validation |
Cosmetic |
NOTOK |
338 |
Should this not be synchronized to avoid multiple callers beginning the construction of a new Bitarchive object |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/TrivialArcRepositoryClient.java', revision 635
Lines |
Description |
Classification |
Status |
52, 56, 58-60 |
Javadoc. |
Cosmetic |
NOTOK |
76 |
Add ArgumentNotValid check for file, and file existence |
Cosmetic |
NOTOK |
90 |
make ArgumentNotValid checks for arcfile (Not Null Not empty), and index (not negative) |
Cosmetic |
NOTOK |
123 |
Note that this argument is not used in this implementation |
Cosmetic |
NOTOK |
128 |
check for Argument not valid? |
Cosmetic |
NOTOK |
133 |
Only need to write "batch" once |
Cosmetic |
NOTOK |
143 |
Check for ArgumentNotValid. |
Cosmetic |
NOTOK |
147-148 |
Remove uncommented code |
Cosmetic |
NOTOK |
163 |
Write "batch job" instead of just "batch" |
Cosmetic |
NOTOK |
188-190 |
Throw an NotImplementedException |
Cosmetic |
NOTOK |
199-200 |
Throw an NotImplementedException |
Cosmetic |
NOTOK |
213-214 |
check for ArgumentNotValid. |
Cosmetic |
NOTOK |
215 |
note in the javadoc, that these variables are not used |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveAdmin.java', revision 774
Lines |
Description |
Classification |
Status |
52 |
Javadoc. |
Cosmetic |
NOTOK |
164, 343 |
use instead files.toArray(new File[files.size]) |
Cosmetic |
NOTOK |
164 |
Too long line |
Cosmetic |
NOTOK |
262 |
Add meaning that the given File object exists, is a directory, and is writable by this process. |
Cosmetic |
NOTOK |
298-305 |
The 'j' is not used explicit -> change to "for (File file : filesHere)" and use 'file' instead of 'filesHere[j]'. |
Cosmetic |
NOTOK |
300 |
replace 'filesHere[j]' with "filesHere[j].getAbsolutePath()"? |
Cosmetic |
NOTOK |
319 |
Validate regexp as Not null and Not empty String |
Cosmetic |
NOTOK |
329-330 |
Replace "Non-file" with "Not a regular File" |
Cosmetic |
NOTOK |
364 |
"Corrupt" => "Possibly corrupt" |
Cosmetic |
NOTOK |
370 |
Consider logging at trace |
Cosmetic |
NOTOK |
388-395 |
use for (File file : files) { ... } |
Cosmetic |
NOTOK |
404-412 |
use for (File file : files) { ... } |
Cosmetic |
NOTOK |
421-429 |
use for (File file : files) { ... } |
Cosmetic |
NOTOK |
471-474 |
Different messages. |
Cosmetic |
NOTOK |
479-480 |
Why use two line for this? |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/LocalArcRepositoryClient.java', revision 554
Lines |
Description |
Classification |
Status |
General |
Should this also read the MINSPACELEFT setting used by the Bitarchives? Add as TODO |
Cosmetic |
NOTOK |
71-72 |
Javadoc. |
Cosmetic |
NOTOK |
79 |
Add a log.info message, that the DEFAULT_DIR_NAME is included as one of the fileDirs |
Cosmetic |
NOTOK |
105 |
throws also IllegalState. Either add IllegalState, or remove the others. |
Cosmetic |
NOTOK |
175 |
Mention that the location argument is unused in this implementation |
Cosmetic |
NOTOK |
225 |
Perhaps also check for '!filesInDir.isEmpty' to aviod making a unnecessary call to the 'files.addAll' method. |
Cosmetic |
NOTOK |
248 |
Remove uncommented code |
Cosmetic |
NOTOK |
260-262 |
Throw NotImplementedException, and add note about this to javadoc |
Cosmetic |
NOTOK |
271-272 |
Throw NotImplementedException and add note about this to javadoc |
Cosmetic |
NOTOK |
279-280 |
Add that bitarchiveName is unused in this implementation of ArcRepositoryClient And is therefore not checked |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/common/utils/arc/ARCBatchJob.java', revision 544
Lines |
Description |
Classification |
Status |
45 |
remove empty line |
Cosmetic |
NOTOK |
97 |
"if file" => if file was |
Cosmetic |
NOTOK |
135-140 |
This is catched by the outer try-catch, and handled exactly the same way. Remove try-catch |
Minor |
NOTOK |
139 |
Add comment: "This aborts the processing of this ARC-file" |
Cosmetic |
NOTOK |
160 |
log here: Unexpected exception caught. The processing of the rest of the arcrecords aborted |
Cosmetic |
NOTOK |
193-195 |
Javadoc this, and argument not valid (or describe why not). |
Cosmetic |
NOTOK |
206, 209 |
Are we supposed to write the 'throws ArgumentNotValid' for all the methods, which makes the ArgumentNotValid check? NO Because then it is needed in a lot of methods. |
Cosmetic |
NOTOK |
208 |
Check 'ArgumentNotValid' on 'arcfile' (checkNotNull) and 'index' (checkNotNegative). |
Cosmetic |
NOTOK |
236-238 |
Javadoc |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/LoadableFileBatchJob.java', revision 548
Lines |
Description |
Classification |
Status |
42 |
Javadoc. |
Cosmetic |
NOTOK |