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

IssuesFoundInReviewNs27 (last edited 2010-08-16 10:24:30 by localhost)