Sqoop
  1. Sqoop
  2. SQOOP-841

Sqoop2: Remove final keyword from manager classes

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 1.99.1
    • Fix Version/s: 1.99.2
    • Component/s: None
    • Labels:
      None

      Description

      I've made all manager classes final in SQOOP-618 and consequently made them singleton objects in SQOOP-802. I believe that the final keyword is no longer required. Removing it will also simplify testing.

      1. bugSQOOP-841.patch
        2 kB
        Jarek Jarcec Cecho

        Issue Links

          Activity

          Hide
          Venkat Ranganathan added a comment -

          +1 - It really helps with testing.

          I am thinking on the same topic of singletons, we can make the constructors private. I can open a JIRA and provide a patch for this if we are OK with that. Not sure whether we want setInstance() also as that seems to violate the singleton contract (though I understand the intent is to use for tests)

          Thanks

          Show
          Venkat Ranganathan added a comment - +1 - It really helps with testing. I am thinking on the same topic of singletons, we can make the constructors private. I can open a JIRA and provide a patch for this if we are OK with that. Not sure whether we want setInstance() also as that seems to violate the singleton contract (though I understand the intent is to use for tests) Thanks
          Hide
          Jarek Jarcec Cecho added a comment -

          Hi Venkat,
          thank you very much for your opinion. I'm glad that you agree with my proposal!

          I was planning to subclass the managers in tests to spoof out the functionality and having the constructor to private might prohibit me doing that. However I have to agree that having the constructor and setInstance() method public is not the best approach either :-/

          Jarcec

          Show
          Jarek Jarcec Cecho added a comment - Hi Venkat, thank you very much for your opinion. I'm glad that you agree with my proposal! I was planning to subclass the managers in tests to spoof out the functionality and having the constructor to private might prohibit me doing that. However I have to agree that having the constructor and setInstance() method public is not the best approach either :-/ Jarcec
          Hide
          Venkat Ranganathan added a comment -

          Hi Jarcec

          I understand your predicament May be something else can be worked out but as these are internal classes, hopefully, the methods will not be misused

          Show
          Venkat Ranganathan added a comment - Hi Jarcec I understand your predicament May be something else can be worked out but as these are internal classes, hopefully, the methods will not be misused
          Hide
          Jarek Jarcec Cecho added a comment -

          Yeah I share your concern Venkat. The setInstance() method can be too easily misused :-/

          Show
          Jarek Jarcec Cecho added a comment - Yeah I share your concern Venkat. The setInstance() method can be too easily misused :-/
          Hide
          Cheolsoo Park added a comment -

          +1. I will commit it after running tests.

          Show
          Cheolsoo Park added a comment - +1. I will commit it after running tests.
          Hide
          Cheolsoo Park added a comment -

          Committed to sqoop2. Thanks Jarcec!

          Show
          Cheolsoo Park added a comment - Committed to sqoop2. Thanks Jarcec!
          Hide
          Hudson added a comment -

          Integrated in Sqoop2-hadoop200 #171 (See https://builds.apache.org/job/Sqoop2-hadoop200/171/)
          SQOOP-841: Remove final keyword from manager classes (Revision 554e5aeffea7e05c0fb3a3d620167a68de781390)

          Result = SUCCESS
          cheolsoo : https://git-wip-us.apache.org/repos/asf?p=sqoop.git&a=commit&h=554e5aeffea7e05c0fb3a3d620167a68de781390
          Files :

          • core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java
          • core/src/main/java/org/apache/sqoop/framework/FrameworkManager.java
          • core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java
          • core/src/main/java/org/apache/sqoop/repository/RepositoryManager.java
          Show
          Hudson added a comment - Integrated in Sqoop2-hadoop200 #171 (See https://builds.apache.org/job/Sqoop2-hadoop200/171/ ) SQOOP-841 : Remove final keyword from manager classes (Revision 554e5aeffea7e05c0fb3a3d620167a68de781390) Result = SUCCESS cheolsoo : https://git-wip-us.apache.org/repos/asf?p=sqoop.git&a=commit&h=554e5aeffea7e05c0fb3a3d620167a68de781390 Files : core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java core/src/main/java/org/apache/sqoop/framework/FrameworkManager.java core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java core/src/main/java/org/apache/sqoop/repository/RepositoryManager.java
          Hide
          Hudson added a comment -

          Integrated in Sqoop2-hadoop100 #157 (See https://builds.apache.org/job/Sqoop2-hadoop100/157/)
          SQOOP-841: Remove final keyword from manager classes (Revision 554e5aeffea7e05c0fb3a3d620167a68de781390)

          Result = SUCCESS
          cheolsoo : https://git-wip-us.apache.org/repos/asf?p=sqoop.git&a=commit&h=554e5aeffea7e05c0fb3a3d620167a68de781390
          Files :

          • core/src/main/java/org/apache/sqoop/repository/RepositoryManager.java
          • core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java
          • core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java
          • core/src/main/java/org/apache/sqoop/framework/FrameworkManager.java
          Show
          Hudson added a comment - Integrated in Sqoop2-hadoop100 #157 (See https://builds.apache.org/job/Sqoop2-hadoop100/157/ ) SQOOP-841 : Remove final keyword from manager classes (Revision 554e5aeffea7e05c0fb3a3d620167a68de781390) Result = SUCCESS cheolsoo : https://git-wip-us.apache.org/repos/asf?p=sqoop.git&a=commit&h=554e5aeffea7e05c0fb3a3d620167a68de781390 Files : core/src/main/java/org/apache/sqoop/repository/RepositoryManager.java core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java core/src/main/java/org/apache/sqoop/framework/FrameworkManager.java

            People

            • Assignee:
              Jarek Jarcec Cecho
              Reporter:
              Jarek Jarcec Cecho
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development