Differences between revisions 2 and 4 (spanning 2 versions)
Revision 2 as of 2009-03-20 13:52:47
Size: 4366
Comment:
Revision 4 as of 2010-08-16 10:24:42
Size: 4300
Editor: localhost
Comment: converted to 1.6 markup
Deletions are marked like this. Additions are marked like this.
Line 9: Line 9:
Line 12: Line 11:
Line 15: Line 13:
SingleMBeanObject, r770, Lines 193,198, 219, 222-228  SingleMBeanObject, r770, Lines 193,198, 219, 222-228
Line 19: Line 17:
FileUtils, r770, Lines 303, 309-312, 419-420, 427-431, 528-529, 533-536, 958-959, 964-967, 987, 992-995  FileUtils, r770, Lines 303, 309-312, 419-420, 427-431, 528-529, 533-536, 958-959, 964-967, 987, 992-995
Line 21: Line 19:

Line 25: Line 21:
{{{  {{{
Line 29: Line 25:
{{{ 
{{{
Line 34: Line 31:
Line 43: Line 39:
|| 168-170 || log? || Cosmetic || NOTOK ||
|| 199-200 || log? || Cosmetic || NOTOK ||
|| 233-234 || log? || Cosmetic || NOTOK ||
|| 168-170 || log? || Cosmetic || OK ||
|| 199-200 || log? || Cosmetic || OK ||
|| 233-234 || log? || Cosmetic || OK ||
Line 48: Line 44:
|| 218 || Make ArgumentNotValid - check on FileBatchJob. || Cosmetic || NOTOK || || 218 || Make ArgumentNotValid - check on FileBatchJob. || Cosmetic || OK ||
Line 53: Line 49:
|| 40 || Should this be public or private? || Cosmetic || NOTOK ||
|| 42, 44 || Are these variables supposed to be public? || Cosmetic || NOTOK ||
|| 59 || Check arguments || Cosmetic || NOTOK ||
|| 40 || Should this be public or private? || Cosmetic || OK ||
|| 42, 44 || Are these variables supposed to be public? || Cosmetic || OK ||
|| 59 || Check arguments || Cosmetic || OK ||
Line 58: Line 54:
|| 153 || The variablename "i" is usually used as an integer for going through the "index" in for-loops. Could be replaced with "iterator" or just "it". || Cosmetic || NOTOK ||
|| 156-159 || Implement the wanted feature in the TODO. This is already a feature request. Refer to this in the comments. || Cosmetic || NOTOK ||
|| 160-161 || Slå disse if-statements sammen til: if(checkArchiveDir(dir) && (bytesFreeInDir > minSpaceLeft + requestedSize)) || Minor || NOTOK ||
|| 153 || The variablename "i" is usually used as an integer for going through the "index" in for-loops. Could be replaced with "iterator" or just "it". || Cosmetic || OK ||
|| 156-159 || Implement the wanted feature in the TODO. This is already a feature request. Refer to this in the comments. || Cosmetic || OK ||
|| 160-161 || Slå disse if-statements sammen til: if(checkArchiveDir(dir) && (bytesFreeInDir > minSpaceLeft + requestedSize)) || Minor || OK ||
Line 63: Line 59:
|| 310-311 || Log? || Cosmetic || NOTOK ||
|| 428-429 || log? || Cosmetic || NOTOK ||
|| 534-535 || log? || Cosmetic || NOTOK ||
|| 965-966 || log? || Cosmetic || NOTOK ||
|| 993-994 || log? || Cosmetic || NOTOK ||
|| 310-311 || Log? || Cosmetic || OK ||
|| 428-429 || log? || Cosmetic || OK ||
|| 534-535 || log? || Cosmetic || OK ||
|| 965-966 || log? || Cosmetic || OK ||
|| 993-994 || log? || Cosmetic || OK ||
Line 70: Line 66:
|| 61 || use val.isEmpty() || Cosmetic || NOTOK ||
|| 74 || Wrong order => public static void || Cosmetic || NOTOK ||
|| 88, 102 || Why use 'i' when standard integer and 'num' when long integer? || Cosmetic || NOTOK ||
|| 61 || use val.isEmpty() || Cosmetic || OK ||
|| 74 || Wrong order => public static void || Cosmetic || OK ||
|| 88, 102 || Why use 'i' when standard integer and 'num' when long integer? || Cosmetic || OK ||

Review (NS-31): Review of bugfix for bug 1564: Disk mount filled to last block - and then it's not possible to move a single file

Author

Søren

Moderator

Søren

State

Closed

Objectives

Review of bugfix for bug 1564.
Review of the fixed createBitarchiveAppId(). Now uses the CommonSettings.APPLICATION_INSTANCE_ID setting (if set), instead of CommonSettings.HTTP_PORT_NUMBER to distinguish between multiple apps on the same machine
Review minor fixes to build.xml (added derbytools library to the classpath written to the MANIFEST.MF),
ArgumentNotValid (corrected method modifier order), GetDataResolver(better logging), FileUtils (better error-checking), and DomainIngester (javadoc added)
BitarchiveAdmin, r752, Lines 136-177
BitarchiveServer, r752, Lines 455-500
SingleMBeanObject, r770, Lines 193,198, 219, 222-228
Bitarchive, r752, Lines 220-221
build.xml, r762, Lines 35
ArgumentNotValid, r763, Lines 27-164
FileUtils, r770, Lines 303, 309-312, 419-420, 427-431, 528-529, 533-536, 958-959, 964-967, 987, 992-995
GetDataResolver, r770, Lines 168-169, 199-200, 233-234

Summary

Follow up by SVC

Total Time Used (Coding,Documentation,Review):

Time use (Coding,Documentation,Review)
SVC: 1.5 md
JOLF: 0.5 md

General comments:

Description

Classification

Status

Comments on file 'trunk/src/dk/netarkivet/common/management/SingleMBeanObject.java', revision 770

Lines

Description

Classification

Status

Comments on file 'trunk/src/dk/netarkivet/viewerproxy/GetDataResolver.java', revision 770

Lines

Description

Classification

Status

168-170

log?

Cosmetic

OK

199-200

log?

Cosmetic

OK

233-234

log?

Cosmetic

OK

Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/Bitarchive.java', revision 752

Lines

Description

Classification

Status

218

Make ArgumentNotValid - check on FileBatchJob.

Cosmetic

OK

Comments on file 'trunk/build.xml', revision 762

Lines

Description

Classification

Status

Comments on file 'trunk/src/dk/netarkivet/harvester/webinterface/DomainIngester.java', revision 764

Lines

Description

Classification

Status

40

Should this be public or private?

Cosmetic

OK

42, 44

Are these variables supposed to be public?

Cosmetic

OK

59

Check arguments

Cosmetic

OK

Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveAdmin.java', revision 752

Lines

Description

Classification

Status

153

The variablename "i" is usually used as an integer for going through the "index" in for-loops. Could be replaced with "iterator" or just "it".

Cosmetic

OK

156-159

Implement the wanted feature in the TODO. This is already a feature request. Refer to this in the comments.

Cosmetic

OK

160-161

Slå disse if-statements sammen til: if(checkArchiveDir(dir) && (bytesFreeInDir > minSpaceLeft + requestedSize))

Minor

OK

Comments on file 'trunk/src/dk/netarkivet/common/utils/FileUtils.java', revision 770

Lines

Description

Classification

Status

310-311

Log?

Cosmetic

OK

428-429

log?

Cosmetic

OK

534-535

log?

Cosmetic

OK

965-966

log?

Cosmetic

OK

993-994

log?

Cosmetic

OK

Comments on file 'trunk/src/dk/netarkivet/common/exceptions/ArgumentNotValid.java', revision 763

Lines

Description

Classification

Status

61

use val.isEmpty()

Cosmetic

OK

74

Wrong order => public static void

Cosmetic

OK

88, 102

Why use 'i' when standard integer and 'num' when long integer?

Cosmetic

OK

Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveServer.java', revision 752

Lines

Description

Classification

Status

IssuesFoundInReviewNs31 (last edited 2010-08-16 10:24:42 by localhost)