= 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 || OK ||
|| 164 || Spell CWD out: Current working directory || Cosmetic || OK ||
|| 202, 204 || public or private variables? || Cosmetic ||POSTPONE ||
|| 205-206 || What is meant by this comment?  Add TODO: Use the HelpFormatter class to print out Usage information || Cosmetic || OK ||
|| 215, 217, 220, 224, 227, 234, 237 || Consider replacing by constants || Cosmetic || OK ||
|| 220-221 || Indentation || Cosmetic || OK ||
|| 222 || Odd place for an empty line. || Cosmetic || OK ||
|| 241 || Javadoc. public/private/protected not defined for this method? || Cosmetic || POSTPONE ||
|| 252 || Javadoc. public/private/protected not defined for this method? || Cosmetic || POSTPONE ||
|| 266-267 || Two empty lines in a row. || Cosmetic || OK ||
|| 268-269 || Shouldn't variables be placed next to each other, and not randomly in the middle of the methods? || Cosmetic || OK ||
|| 340 || change "," to a constant. || Cosmetic || OK ||
|| 355 || Make into bug or feature request (if not already) and refer to this. Or implement || Cosmetic || OK ||
|| 505-506 || Two empty lines in a row || Cosmetic || OK ||
=== 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 || OK ||
|| 109 || ".*" as a constant (static final) variable (same constant as previous) || Cosmetic || OK ||
|| 138 || Make an OR_SEPARATOR constant for "|" || Cosmetic || POSTPONE ||
|| 164, 167 || It does not need to be ARC-files we're processing || Cosmetic || OK ||
|| 174-175, 177-178 || It does not need to be ARC-files we're processing || Cosmetic || OK ||
|| 206, 222 || Log the case, if maxExceptions have been reached || Cosmetic || OK ||
|| 235 || Log the case, if maxExceptions have been reached || Cosmetic || OK ||
|| 259 || Add a TODO, that this should/could be a setting || Cosmetic || OK ||
|| 330 || Replace -1 with UNKNOWN_OFFSET || Cosmetic || OK ||
|| 393-398 || Javadoc this method. || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/BatchFilter.java', revision 548 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| General || Rename BatchFilter as ARCBatchFilter. || Cosmetic || OK ||
|| 117 || Make comment about 'mimetypeIsOk' checks for ArgumentNotValid, or check the mimetype for ArgumentNotValid. || Cosmetic || OK ||
=== 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 || POSTPONE ||
=== Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/BatchLocalFiles.java', revision 548 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 43 || javadoc || Cosmetic || OK ||
|| 49 || Replace "used for batching" by "processed by the batch job" || Cosmetic || OK ||
|| 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 || OK ||
|| 84 || Should we add the initialization exception to the list of exceptions for job? Add as TODO || Cosmetic || OK ||
|| 90 || Should we add this finish exception to the list of exceptions for job? Add as TODO || Cosmetic || OK ||
|| 108-109 || Should we add this exception thrown during processing to the list of exceptions for job? Add as TODO || Cosmetic || OK ||
=== 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 || POSTPONE ||
|| 74 || Rename con as jmscon || Cosmetic || POSTPONE ||
|| 120 || Consider checking that inbMsg is not null || Cosmetic || POSTPONE ||
|| 144 || Consider checking that beMsg is not null || Cosmetic || POSTPONE ||
|| 176 || Consider checking that hbMsg is not null || Cosmetic || POSTPONE ||
|| 192 || 'o' is not a very good variable name og an observable object. Use 'obs' or equivilent; || Cosmetic || POSTPONE ||
|| 202, 206, 212 || Add respectively "an", "an", and "a" after "Received" || Cosmetic || POSTPONE ||
=== 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 || POSTPONE ||
|| 48 || Rename "BA_ApplicationId" as "BitarchiveApplicationId" || Cosmetic || POSTPONE ||
|| 59-61 || three empty lines in a row. Delete two of them. || Cosmetic || POSTPONE ||
|| 73 || rename to BitarchiveApplicationId || Cosmetic || POSTPONE ||
|| 76 || "he remote file" => "The remote file" || Cosmetic || POSTPONE ||
|| 177 || Add ArgumentNotValid check || Cosmetic || POSTPONE ||
=== 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 || OK ||
|| 119 || Replace "No data loaded!!!!!" with "No data loaded for class with name '" +  className + "'." || Cosmetic || OK ||
|| 157 || Check argument os || Cosmetic || OK ||
|| 170 || 'Cannout' = spelling error. || Cosmetic || OK ||
|| 192 || "Finish up" => "Finish" || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/BatchStatus.java', revision 544 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 35 || Delete line || Cosmetic || POSTPONE ||
|| 58-59 || ".., that run this batchJob" => ".., that ran this batch job" || Cosmetic || POSTPONE ||
|| 66-70 || Check for ArgumentNotValid. || Cosmetic || POSTPONE ||
|| 90 || Make "ALL_BITARCHIVE_APPS" a constant || Cosmetic || POSTPONE ||
|| 160 || ".. to copy into '" => ".. to copy into target file '" || Cosmetic || POSTPONE ||
|| 161 || "batch job on" => "batch job run on " || Cosmetic || POSTPONE ||
|| 168 || "so this" => so this method || Cosmetic || POSTPONE ||
|| 187 || the variable 'stream' cannot be printed out. Just write "append to output stream". || Cosmetic || POSTPONE ||
|| 188 || "batch job on" => batch job run on bitarchive application || Cosmetic || POSTPONE ||
=== 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 ||POSTPONE ||
|| 59 || Remove superfluous whitespace || Cosmetic || POSTPONE ||
|| 75 || Remove superfluous whitespace || Cosmetic || POSTPONE ||
|| 101-105 || replace with: ArgumentNotValid.checkTrue(arcFileDirectory.isDirectory() && arcFileDirectory.canRead(), "..."); || Cosmetic || POSTPONE ||
|| 106-110 || replace with: ArgumentNotValid.checkTrue(cdxFileDirectory.isDirectory() && cdxFileDirectory.canRead(), "..."); || Cosmetic || POSTPONE ||
|| 140 || could be written if (!exceptions.isEmpty()) || Cosmetic || POSTPONE ||
=== 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 || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/Bitarchive.java', revision 774 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 106 || ArgumentNotValid.checkNotNegative on 'index' ? || Cosmetic ||POSTPONE ||
|| 112 || don't use 'arcfile' before it is checked for 'ArgumentNotValid' on the following line. || Cosmetic || POSTPONE ||
|| 136-140 || Remove uncommented code || Cosmetic || POSTPONE ||
|| 183 || check for ArgumentNotValid before use of argument-variables. || Cosmetic || POSTPONE ||
|| 261-264 || Identical messages, use a string. || Cosmetic || POSTPONE ||
|| 318 || Move log after argument validation || Cosmetic || POSTPONE ||
|| 338 || Should this not be synchronized to avoid multiple callers beginning the construction of a new Bitarchive object || Cosmetic || POSTPONE ||
=== Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/TrivialArcRepositoryClient.java', revision 635 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 52, 56, 58-60 || Javadoc. || Cosmetic || OK ||
|| 76 || Add ArgumentNotValid check for file, and file existence || Cosmetic || OK ||
|| 90 || make ArgumentNotValid checks for arcfile (Not Null Not empty), and index (not negative) || Cosmetic || OK ||
|| 123 || Note that this argument is not used in this implementation || Cosmetic || OK ||
|| 128 || check for Argument not valid? || Cosmetic || OK ||
|| 133 || Only need to write "batch" once || Cosmetic || OK ||
|| 143 || Check for ArgumentNotValid. || Cosmetic || OK ||
|| 147-148 || Remove uncommented code || Cosmetic || OK ||
|| 163 || Write "batch job" instead of just "batch" || Cosmetic || OK ||
|| 188-190 || Throw an NotImplementedException || Cosmetic || OK ||
|| 199-200 || Throw an NotImplementedException || Cosmetic || OK ||
|| 213-214 || check for ArgumentNotValid. || Cosmetic || OK ||
|| 215 || note in the javadoc, that these variables are not used || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveAdmin.java', revision 774 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 52 || Javadoc. || Cosmetic || OK ||
|| 164, 343 || use instead files.toArray(new File[files.size]) || Cosmetic || OK ||
|| 164 || Too long line || Cosmetic || OK ||
|| 262 || Add meaning that the given File object exists, is a directory, and is writable by this process. || Cosmetic || OK ||
|| 298-305 || The 'j' is not used explicit -> change to "for (File file : filesHere)" and use 'file' instead of 'filesHere[j]'. || Cosmetic || OK ||
|| 300 || replace 'filesHere[j]' with "filesHere[j].getAbsolutePath()"? || Cosmetic || OK ||
|| 319 || Validate regexp as Not null and Not empty String || Cosmetic || OK ||
|| 329-330 || Replace "Non-file" with "Not a regular File" || Cosmetic || OK ||
|| 364 || "Corrupt" => "Possibly corrupt" || Cosmetic || OK ||
|| 370 || Consider logging at trace || Cosmetic || OK ||
|| 388-395 || use for (File file : files)  {   ...  } || Cosmetic || OK ||
|| 404-412 || use for (File file : files)  {   ...  } || Cosmetic || OK ||
|| 421-429 || use for (File file : files)  {   ...  } || Cosmetic || OK ||
|| 471-474 || Different messages. || Cosmetic ||POSTPONE ||
|| 479-480 || Why use two line for this? || Cosmetic || OK ||
=== 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 || POSTPONE ||
|| 71-72 || Javadoc. || Cosmetic || POSTPONE ||
|| 79 || Add a log.info message, that the DEFAULT_DIR_NAME is included as one of the fileDirs || Cosmetic || POSTPONE ||
|| 105 || throws also IllegalState. Either add IllegalState, or remove the others. || Cosmetic || POSTPONE ||
|| 175 || Mention that the location argument is unused in this implementation || Cosmetic || POSTPONE ||
|| 225 || Perhaps also check for '!filesInDir.isEmpty' to aviod making a unnecessary call to the 'files.addAll' method. || Cosmetic ||POSTPONE ||
|| 248 || Remove uncommented code || Cosmetic || POSTPONE ||
|| 260-262 || Throw NotImplementedException, and add note about this to javadoc || Cosmetic ||POSTPONE ||
|| 271-272 || Throw NotImplementedException and add note about this to javadoc || Cosmetic || POSTPONE ||
|| 279-280 || Add that bitarchiveName is unused in this implementation of ArcRepositoryClient And is therefore not checked || Cosmetic || POSTPONE ||
=== Comments on file 'trunk/src/dk/netarkivet/common/utils/arc/ARCBatchJob.java', revision 544 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 45 || remove empty line || Cosmetic || POSTPONE ||
|| 97 || "if file" => if file was || Cosmetic || POSTPONE ||
|| 135-140 || This is catched by the outer try-catch, and handled exactly the same way. Remove try-catch || Minor || OK ||
|| 139 || Add comment: "This aborts the processing of this ARC-file" || Cosmetic ||REJECTED ||
|| 160 || log here: Unexpected exception caught. The processing of the rest of the arcrecords aborted || Cosmetic || POSTPONE ||
|| 193-195 || Javadoc this, and argument not valid (or describe why not). || Cosmetic || OK ||
|| 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 || OK ||
|| 208 || Check 'ArgumentNotValid' on 'arcfile' (checkNotNull) and 'index' (checkNotNegative). || Cosmetic || REJECTED ||
|| 236-238 || Javadoc || Cosmetic || OK ||
=== Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/LoadableFileBatchJob.java', revision 548 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 42 || Javadoc. || Cosmetic || POSTPONE ||