Differences between revisions 1 and 4 (spanning 3 versions)

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

-  ⇤ ← Revision 1 as of 2009-03-19 18:01:42 → 
  Size: 4217
  Editor: SoerenCarlsen
  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 24:
+'''Total Time Used (Coding,Documentation,Review)''':

{{{
Time use (Coding,Documentation,Review)
SVC: 1.5 md
JOLF: 0.5 md
}}}
-Line 26:
+Line 33:
-Line 30:
+Line 39:
-|| 199-200 || log? || Cosmetic || NOTOK ||
|| 233-234 || log? || Cosmetic || NOTOK ||
|| 168-170 || log? || Cosmetic || NOTOK ||
+|| 168-170 || log? || Cosmetic || OK ||
|| 199-200 || log? || Cosmetic || OK ||
|| 233-234 || log? || Cosmetic || OK ||
-Line 35:
+Line 44:
-|| 218 || Make ArgumentNotValid - check on FileBatchJob. || Cosmetic || NOTOK ||
+|| 218 || Make ArgumentNotValid - check on FileBatchJob. || Cosmetic || OK ||
-Line 40:
+Line 49:
-|| 59 || Check arguments || Cosmetic || NOTOK ||
|| 42, 44 || Are these variables supposed to be public? || Cosmetic || NOTOK ||
|| 40 || Should this be public or private? || 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 45:
+Line 54:
-|| 160-161 || Slå disse if-statements sammen til: if(checkArchiveDir(dir) && (bytesFreeInDir > minSpaceLeft + requestedSize)) || Minor || NOTOK ||
|| 156-159 || Implement the wanted feature in the TODO. This is already a feature request. Refer to this in the comments. || Cosmetic || 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 || 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 50:
+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 57:
+Line 66:
-|| 74 || Wrong order => public static void || Cosmetic || NOTOK ||
|| 61 || use val.isEmpty() || 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 ||