Uploaded image for project: 'NetarchiveSuite'
  1. NetarchiveSuite
  2. NAS-23

Refactor ChannelId access

    XMLWordPrintable

Details

    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

          Activity

            People

              mss Mikis Seth Sørensen (Inactive)
              mss Mikis Seth Sørensen (Inactive)
              Søren Vejrup Carlsen Søren Vejrup Carlsen (Inactive)
              Watchers:
              0 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Time Tracking

                  Estimated:
                  Original Estimate - 35h
                  35h
                  Remaining:
                  Remaining Estimate - 0h
                  0h
                  Logged:
                  Time Spent - 15h Time Not Required
                  15h