Review (NS-147): Archive Assignment B.2.2 complete

Author	Jonas
Moderator	Jonas
State	Closed

Objectives

Besides containing all the latest work on database archival, it also contains a lot of code style changes.

Summary

Follow up by JOLF

Total Time Used (Coding,Documentation,Review):

Time use (Coding,Documentation,Review)
JOLF: 8 MD
SVC: 2 MD

General comments:

Description	Classification	Status
Java logging is used a couple of places in the Deploy code. And we're not running DeployApplication with a log.properties assigned to it. Consider using the same log.prop when running the DeployApplication, so the any DeployApplication logging is written to logs: -Dorg.apache.commons.logging.Log=org.apache.commons.logging.impl.Jdk14Logger -Djava.util.logging.config.file=/home/test/SVC/conf/log_ArcRepositoryApplication.prop	Cosmetic	POSTPONE
Our use of the word arcrepository indicates the storage of arcfiles. Consider renaming "arcrepository" as "repository"	Cosmetic	POSTPONE

Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumFileServer.java', revision 1320

Lines	Description	Classification	Status
161	make "_" as a constant	Cosmetic	OK
180	Spelling: Receving => Receiving	Cosmetic	OK
300	Spelling: Recieving => Receiving	Cosmetic	OK
338	Spelling: Receving => Receiving	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/ReplicaFileInfo.java', revision 1283

Lines	Description	Classification	Status
72	Add validation: fDate/cDate (not null), the String values (not null not empty), the int/long values (positive integers)	Cosmetic	OK
164	rename method: getFileListCheckdatetime()	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveMonitor.java', revision 1283

Lines	Description	Classification	Status
237	That depends on the semantics, we want to impose: If exceptions is null means no exceptions, null is ok; otherwise we should disallow null; and enforce an empty list meaning no exceptions. In anycase the javadoc for BitarchiveReply should describe our chosen semantics	Cosmetic	INVALID
312	spelling: live => alive	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/ReplicaCacheDatabase.java', revision 1311

Lines	Description	Classification	Status
125	ID => identifiers	Cosmetic	OK
348	Add to logentry and to javadoc: Returning the first one	Cosmetic	OK
378	Add: Returning the first one	Cosmetic	OK
877	Implement TODO, or remove TODO	Cosmetic	OK
1386	Dangerous use of ==. May or not work as intended Use instead if (us.equals(ReplicaStoreState.UPLOAD_COMPLETED))	Cosmetic	OK
1788	This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc	Cosmetic	OK
1821	This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc	Cosmetic	OK
1989	spelling: bitarche => bitarchive	Cosmetic	OK
2061	Missing validation of argument 'line'	Cosmetic	OK
2079-2080	Remove?	Cosmetic	OK
2132	Add null validation of 'date' argument	Cosmetic	OK
2160	Can we kill this method now, or should we wait to the next stable release? No, just kill FIXME comment!	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/settings.xml', revision 1300

Lines

Description

Classification

Status

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/JMSArcRepositoryClient.java', revision 1295

Lines	Description	Classification	Status
537	Spelling: Recieved => Received	Cosmetic	OK
597	Spelling: Recieved => Received	Cosmetic	OK
657	Spelling: Recieved => Received	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/deploy/LinuxMachine.java', revision 1305

Lines	Description	Classification	Status
1316-1318	Explain, that the output of shutdown is extended to the same file as the output when starting the database.	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/tools/ReestablishAdminDatabase.java', revision 1311

Lines	Description	Classification	Status
General	missing SVN:keywords	Cosmetic	OK
145	Make into local constant or include with dk.netarkivet.archive.Constants	Cosmetic	OK
165	spelling: occured => occurred	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/deploy/ScriptConstants.java', revision 1305

Lines	Description	Classification	Status
70	Please rename to OPTION_SECURITY_POLICY	Cosmetic	OK
72	Please rename to OPTION_SECURITY_POLICY_WIN	Cosmetic	OK
74	This is not an appropriate description.	Cosmetic	OK
81-82	Change to one line	Cosmetic	OK
84-85	Change to one line	Cosmetic	OK
118	change to ETC_PROFILE	Cosmetic	OK
125-128	Replace with calls to StringUtils.repeat	Cosmetic	REJECT
356, 369, 382, 398, 414	These checkNotNulls should be replaced with checkNotNullOrEmpty	Cosmetic	OK
429, 447-449	These checkNotNulls should be replaced with checkNotNullOrEmpty	Cosmetic	OK

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

Lines	Description	Classification	Status
84	replace 'to' with 'ChannelID to'.	Cosmetic	OK
85-86	replace 'baAppId' with 'String baAppId'	Cosmetic	OK
88	replace 'originatingBatchMsgId' with 'String originatingBatchMsgId'	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/UpdateableAdminData.java', revision 1283

Lines	Description	Classification	Status
General	There are multiple arguments in this class named "arcfileName". Rename as "fileName"	Cosmetic	OK
General	All references til "arcrepository" should maybe be just "repository"	Cosmetic	POSTPONE
106	spelling: "ddmin data" => "admin data"	Cosmetic	OK
109, 191, 195	This is used in AdminData.toString method to include there information about known bitarchives. The knownBitarchives could probably now be replaced by a database call	Cosmetic	REJECT

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

Lines	Description	Classification	Status
173	Removal initial \n	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/deploy/Machine.java', revision 1302

Lines	Description	Classification	Status
94, 103	rename "bpdbFileName" as "arcdatabaseFilename"	Cosmetic	OK
681	Rename JMXREMOTE_HERITRIX_PRIVELEGES as JMXREMOTE_HERITRIX_PRIVILEGES	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/CDXOriginCrawlLogIterator.java', revision 1289

Lines	Description	Classification	Status
162	Spelling: matchin=> matching	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/deploy/WindowsMachine.java', revision 1302

Lines	Description	Classification	Status
33	Why has this been added, when it is not used?	Cosmetic	OK
46	rename 'e' as 'root'	Cosmetic	OK
868-869	This indentation looks quite odd. Insert line break.	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/checksum/FileChecksumArchive.java', revision 1320

Lines	Description	Classification	Status
123	Maybe, you should rather write: map (filename -> checksum)	Cosmetic	OK
140	Consider adding 'synchronized' to this method	Cosmetic	OK
341-346	Test whether the file is readable	Cosmetic	OK
360	make as a constant	Cosmetic	OK
400	Spelling: occured => occurred	Cosmetic	OK
529-550, 553-574	Duplicate code here; Have only one method and return a dk.netarkivet.common.utils.KeyValuePair instead	Cosmetic	OK
605	spelling: occured => occurred	Cosmetic	OK
645-657	Replace 'arcFile' with 'file'; this code accepts (and should accept) any kind of files	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DatabaseAdmin.java', revision 1285

Lines	Description	Classification	Status
General	Add missing svn:keywords	Cosmetic	OK
67	Add "synchronized" to the method to prevent synchronization problems	Cosmetic	OK
117	add: uniquely identifying the replica	Cosmetic	OK
141-142	Is this still an outstanding issue? Add FR to handle this issue about storestate validity	Cosmetic	REJECT
295-296	Make a more meaningful text for this exception	Cosmetic	OK
346-348	Can be replaced with just instance = null;	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/CombiningMultiFileBasedCache.java', revision 1283

Lines	Description	Classification	Status
45-46	Or instead of "Inheriting the ": Must implement the java.lang.Comparable interface	NA	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/AdminFactory.java', revision 1283

Lines	Description	Classification	Status
General	add SVN:keywords	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/LocalArcRepositoryClient.java', revision 1321

Lines	Description	Classification	Status
423	Add @throws PermissionDenied if the credentials for correcting files are wrong	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DatabaseBasedActiveBitPreservation.java', revision 1293

Lines	Description	Classification	Status
221	Clarify your TODO. This is also about if or when the checksum information from the replica expires, isn't it?	Cosmetic	OK
436	Add: valid meaning not null and not empty	Cosmetic	OK
463	add: invalid meaning not null; Should we accept not empty filenames here?	Cosmetic	OK

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

Lines	Description	Classification	Status
45, 61, 67, 70	Rename as "filename"	Cosmetic	OK
138-140	Rename as getFilename()	Cosmetic	OK
182	Arcfile => File	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/WorkFiles.java', revision 1288

Lines	Description	Classification	Status
254	Add "." after "below here"	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumClient.java', revision 1298

Lines	Description	Classification	Status
95	Wrong comment: A new instance is now created every time	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/deploy/CreateTestInstance.java', revision 1301

Lines	Description	Classification	Status
139	Change constant COMPLETE_HARVEST_HETRIX_GUI_PORT to COMPLETE_HARVEST_HERITRIX_GUI_PORT	Cosmetic	OK
141	Change constant COMPLETE_HARVEST_HETRIX_JMX_PORT to COMPLETE_HARVEST_HERITRIX_JMX_PORT	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/distribute/IndexRequestServer.java', revision 1296

Lines	Description	Classification	Status
199	Remove line	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/tools/RunBatch.java', revision 1289

Lines	Description	Classification	Status
246-249	Consider removing this comment, as this is not a priority to HelpFormatter instead. And furthermore I don't understand the problem statement "HelpFormater only prints directly". It doesn't seem to be true. If it is a problem with cli-1.0 then it could be solved in the latest cli-release (1.2; released march 19, 2009)	NA	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FilePreservationState.java', revision 1294

Lines	Description	Classification	Status
General	The mention of "bitarchives" should be replaced with replica. This is both an issue regarding javadoc, method-naming, and argument naming (i.e. Replica bitarchive in method getBitarchiveChecksum())	Cosmetic	OK
73	We're not validating the checksumMap, here. Is that on purpose? Validate for null and not empty map	Cosmetic	OK
107	The localisation should probably take place in the ReplicaStoreState class. This is possible if the getAdminBitarchiveStoreState does not return null in the case of "No state", but instead returns a ReplicaStoreState object representing "No state"	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/FileBasedCache.java', revision 1289

Lines	Description	Classification	Status
51	Spelling: The type of cache	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileListJob.java', revision 1289

Lines

Description

Classification

Status

Comments on file 'trunk/src/dk/netarkivet/archive/webinterface/BitpreserveFileState.java', revision 1290

Lines	Description	Classification	Status
86	Consider adding a log.warn message at this point	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DerbySpecifics.java', revision 1285

Lines	Description	Classification	Status
32	Add to javadoc, why this class is so short!	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/FileListStatus.java', revision 1276

Lines	Description	Classification	Status
30	=> This is used by ..	Cosmetic	OK
43-44	certain => specific	Cosmetic	OK

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

Lines	Description	Classification	Status
General	Consider adding validation for the visit() methods in this class	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileBasedActiveBitPreservation.java', revision 1283

Lines	Description	Classification	Status
962	Remove TODO; as we don't intend to implement this method	Cosmetic	OK
975	Remove TODO; as we don't intend to implement this method	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/deploy/Constants.java', revision 1304

Lines	Description	Classification	Status
168, 171, 181	HETRIX => HERITRIX	Cosmetic	OK
325	Rename file? bpdb.jar refers to a bitpreservation database file, and the database is now much more.	Cosmetic	OK
518	Archive database ...	Minor	OK
577, 590	Be more strict: Disallow empty strings as valid scopes	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/ArcRepositoryServer.java', revision 1298

Lines	Description	Classification	Status
241	Is this still true? If not, remove line	Cosmetic	OK
255	Change to "Failed to handle GetAllFilenamesMessage: " ...	Cosmetic	OK
276	Change to "Failed to handle GetAllChecksumsMessage: " ...	Cosmetic	OK
292	Spelling: Recieved => Received	Cosmetic	OK
319	Is this still true? If not remove TODO line	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/RawMetadataCache.java', revision 1310

Lines	Description	Classification	Status
180	Add: Currently does nothing	Cosmetic	OK
185-191, 205	throws IOFailure	Cosmetic	OK
206-210	delete?	Cosmetic	OK
216	Add: Currently does nothing	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DerbyServerSpecifics.java', revision 1283

Lines	Description	Classification	Status
44-49	Javadoc is misleading; Change to: Method for shutting down the database system. Has no effect in this implementation Maybe log a warning about this fact when the method is called.	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DatabasePreservationState.java', revision 1283

Lines	Description	Classification	Status
General	missing license header	Cosmetic	OK
General	Replace all(?) instances of "bitarchive" with "replica" in this class	Cosmetic	OK
General	Consider adding a featurerequest for some of TODOs	Cosmetic	OK
General	Consider replacing == with equals, if not certain that they have equal semantics	Cosmetic	OK
42	rename "file" as "filename"	Cosmetic	OK
92-93	Is this correct behaviour and if yes, document it in the javadoc.	Cosmetic	OK
98	If this is for presentation purposes only, move it to the archive.webinterface package	Cosmetic	OK
134-136	Add to javadoc, that it always returns true	Cosmetic	OK
158	Complete logmessage	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/ArcRepository.java', revision 1298

Lines	Description	Classification	Status
211-217	why make a variable which is only used once? Make the string in the creating of the exception.	Cosmetic	OK
396	Refactor to be a method inside one of the Admin implementations	Cosmetic	POSTPONE

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DBConnect.java', revision 1300

Lines	Description	Classification	Status
114	Will this only work for an external database? If no update javadoc.	Cosmetic	OK

Comments on file 'trunk/webpages/BitPreservation/Bitpreservation-filestatus-checksum.jsp', revision 1288

Lines	Description	Classification	Status
126-129	Too long lines	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/Admin.java', revision 1283

Lines	Description	Classification	Status
General	Add svn:keywords	Cosmetic	OK
45	add newline	Cosmetic	OK
61	add newline	Cosmetic	OK
64-65	Deprecate this method; as in the future it will not be possible to setChecksum	Cosmetic	OK
79	add newline	Cosmetic	OK
86	add newline	Cosmetic	OK
104	add newline	Cosmetic	OK
108	This is not decided in the interface, but in the actual implementation(s)	Cosmetic	OK
116	add newline	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/ArchiveStoreState.java', revision 1283

Lines	Description	Classification	Status
74	throws ArgumentNotValid!	Cosmetic	OK
90	throws ArgumentNotValid	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/PreservationState.java', revision 1283

Lines	Description	Classification	Status
General	Add svn:keywords	Cosmetic	OK

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

Lines	Description	Classification	Status
General	Consider adding validation for the visit methods in this class	Cosmetic	OK