Review (NS-19): Bug 934 - removal of Sidekick from NetarchiveSuite

Author	Søren
Moderator	Søren
State	Closed

Objectives

Review of files:
/trunk/src/dk/netarkivet/harvester/settings.xml (removed isRunning setting after line 145)
/trunk/src/dk/netarkivet/harvester/HarvesterSettings.java 552 (removed constant for isRunning setting at the end)
/trunk/src/dk/netarkivet/harvester/harvesting/distribute/HarvestControllerServer.java, r551, 134-135, 151-212, 284-301, 446-459, 659-661
/trunk/src/dk/netarkivet/harvester/harvesting/distribute/HarvestControllerClient.java, r551,63-64 (removal of wrong javadoc throws clause), 76 (changed modifier order)
/trunk/src/dk/netarkivet/harvester/harvesting/HarvestControllerApplication.java (ALL LINES, all lot of code removed)
/trunk/scripts/simple_harvest/harvest.sh, r551, 137-171
/trunk/scripts/simple_harvest/settings.xml, r552, (removed isRunning, and topLevelDomains (already in defaults)
Note that the following files were deleted i r551:
===================================
/trunk/src/dk/netarkivet/harvester/sidekick/DefaultMonitorHook.java
/trunk/src/dk/netarkivet/harvester/sidekick/HarvestControllerServerMonitorHook.java
/trunk/src/dk/netarkivet/harvester/sidekick/MonitorHook.java
/trunk/src/dk/netarkivet/harvester/sidekick/SideKick.java

Total Time Used (Coding,Documentation,Review):

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

General comments:

Description

Classification

Status

Comments on file 'trunk/scripts/simple_harvest/harvest.sh', revision 551

Lines

Description

Classification

Status

Comments on file 'trunk/scripts/simple_harvest/settings.xml', revision 552

Lines

Description

Classification

Status

Comments on file 'trunk/src/dk/netarkivet/harvester/harvesting/distribute/HarvestControllerServer.java', revision 642

Lines	Description	Classification	Status
121-122	Naming the JMSConnection variable 'con' is confusing. It could just as well refer to other variables, e.g. the HarvestController.	Cosmetic	OK
665	Why not use the function resumeAcceptingJobs() instead of just setting the running to false?	Cosmetic	OK

Comments on file 'trunk/src/dk/netarkivet/harvester/harvesting/HarvestControllerApplication.java', revision 551

Lines

Description

Classification

Status

Comments on file 'trunk/src/dk/netarkivet/harvester/harvesting/distribute/HarvestControllerClient.java', revision 551

Lines	Description	Classification	Status
70-78	I thought that a construction factory ensured that only one instance exists. This function allows several identical HarvestControllerClient instances to exist at the same time. Is it the purpose of this method? And if so then why not allow regular (public) use of the constructor?	Cosmetic	REJECTED
54	It is confusing to see a variable name 'con', since it can refer to several things, e.g. a connection, a controller, etc.	Cosmetic	OK