Details
-
Bug
-
Resolution: Fixed
-
Minor
-
None
-
None
Description
As part of the work to enable Jit dispatching of Harvester jobs by the HarvestSceduler (NARC-11) I have run into a lot of muddled code regarding the lookup of ChannelID's used for the harvester job, which is begging for a major refactored. This includes:
- Duplicated code: A lot of examples of the following code block currently exists:
JobPriority p = job.getPriority(); ChannelID result; switch(p) { case LOWPRIORITY: result = Channels.getAnyLowpriorityHaco(); break; case HIGHPRIORITY: result = Channels.getAnyHighpriorityHaco(); break; default: throw new UnknownID("Job " + job + " has illegal priority " + p); }
This should be collected into reusable code accessed by a ChannelID getChannel(Jobpriority jobPriority) metod.
- The logic for mapping JobPriority is replicated and many times corrupted by evolutionary changes. Ex. what does this code do? (HarvestControllerClientTester#testSendingToCorrectQueue())
ChannelID result1; if (JobPriority.HIGHPRIORITY.toString().equals(JobPriority.LOWPRIORITY.toString())) { result1 = Channels.getAnyLowpriorityHaco(); } else { if (JobPriority.HIGHPRIORITY.toString().equals(JobPriority.HIGHPRIORITY.toString())) { result1 = Channels.getAnyHighpriorityHaco(); } else { throw new UnknownID(JobPriority.HIGHPRIORITY.toString() + " is not a valid priority"); } }
Answer: the same as:
ChannelID result1 = getChannel(Jobpriority.HIGHPRIORITY);
Code like this should be cleanup and the JobPriority -> ChannelId mapping should be encapsulated as describe above.
- Legacy tests: Obsolete tests cases exist, which are now meaningless. Example (ChannelsGetAnyHacoTester#testGetAnyHaco()):
// Test setting high priority in argument ChannelID result4; if ("HIGHPRIORITY".equals(JobPriority.LOWPRIORITY.toString())) { result4 = Channels.getAnyLowpriorityHaco(); } else { if ("HIGHPRIORITY".equals(JobPriority.HIGHPRIORITY.toString())) { result4 = Channels.getAnyHighpriorityHaco(); } else { throw new UnknownID("HIGHPRIORITY" + " is not a valid priority"); } } ChannelID anyHaco1 = result4; assertEquals("Channel should be high priority", env + "_COMMON_ANY_HIGHPRIORITY_HACO", anyHaco1.getName());
Tests like this should be remove as they don't really test anything (they use to though, before the getAnyHaco("") was removed).
- A lot of Harvester channel specific code (same for other modules) exists in the Channels class and tests. This should be moved to appropriate models to enforce localization of concerns to the relevant parts of the component model/modules.
Attachments
Issue Links
- was spawned by
-
NAS-24 Stop using the JMS queues for queuing snapshot harvests
- Resolved