Review (NS-23): FR 1497 (Button in NetarchiveSuite)
Author |
Søren |
Moderator |
Søren |
State |
Closed |
Objectives
Review of code that fixed FR 1497: trunk/src/dk/netarkivet/harvester/harvesting/DirectHeritrixController.java, r537, lines 193-201 trunk/src/dk/netarkivet/harvester/harvesting/HeritrixController.java, r537, lines 131-135 trunk/src/dk/netarkivet/harvester/harvesting/HeritrixLauncher.java, r537, lines 216-219 trunk/src/dk/netarkivet/harvester/harvesting/JMXHeritrixController.java, r537, lines 501-508 trunk/src/dk/netarkivet/monitor/webinterface/JMXSummaryUtils.java, r537, lines 234-235, 242 The primary changes: Addition of getHarvestInformation() method to HeritrixController interface, and its implementation in DirectHeritrixController and JMXHeritrixController. Use this method in a logmessage in HeritrixLauncher.doCrawlLoop() In JMXSummaryUtils.generateMessage, any text found in a logmessage that starts with http are turned into a clickable link.
Summary
SVC to follow-up
Total Time Used (Coding,Documentation,Review):
Time use (Coding,Documentation,Review) KFC: 1 SVC: 0.25 JOLF: 0.25
General comments:
Description |
Classification |
Status |
Comments on file 'trunk/src/dk/netarkivet/harvester/harvesting/DirectHeritrixController.java', revision 537
Lines |
Description |
Classification |
Status |
193-200 |
What if the harvester is not running?? It will still return "running inline", even if does not run. Also shouldn't "running inline" be a put into a constant? Followup: Add a TODO to make this method respond after how the Harvester really is doing, and not just respond "Running inline" |
Cosmetic |
NOTOK |
222-227 |
Fix this indentation. |
Cosmetic |
NOTOK |
141-143 |
No Javadoc on this function. |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/harvester/harvesting/HeritrixController.java', revision 537
Lines |
Description |
Classification |
Status |
132 |
"where to access" => the URL to a the GUI of running harvester |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/monitor/webinterface/JMXSummaryUtils.java', revision 537
Lines |
Description |
Classification |
Status |
47-51, 60-61 |
Javadoc these constants. |
Cosmetic |
NOTOK |
46 |
Missing dot in javadoc |
Cosmetic |
NOTOK |
235 |
This line is too long. Add comment to this line: All strings starting with "http:" or "https:" are replaced with proper HTML Anchor |
Cosmetic |
NOTOK |
286-291 |
Javadoc this. |
Cosmetic |
NOTOK |
Comments on file 'trunk/src/dk/netarkivet/harvester/harvesting/JMXHeritrixController.java', revision 537
Lines |
Description |
Classification |
Status |
252-255, 288-291, 296, 301-303, 308, 318, 325-327, 346, 356, 390, 454 |
These function does something else than just call the HeritrixController#cleanup() function. What? and why not describe it? |
Cosmetic |
OK |
71, 78-86 |
Javadoc these constants and the log. And add TODO: Add feature request in the Heritrix project to make publicly available constants for these attributes in the Heritrix code. |
Cosmetic |
OK |
157-165 |
Also Javadoc these constants. Why are these constants not publicly available in the Heritrix code. Add feature request in the Heritrix project |
Cosmetic |
OK |
507 |
Remove empty line |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/harvester/harvesting/HeritrixLauncher.java', revision 537
Lines |
Description |
Classification |
Status |
50-51 |
Two empty lines next to each other. |
Cosmetic |
OK |