Derby
  1. Derby
  2. DERBY-2412

Refactor NetworkServerControlImpl

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Network Server
    • Labels:
      None

      Description

      NetworkServerControlImpl is overly complex and serves several purposes. This makes it hard to penetrate the logic, to debug and to maintain.

      I propose (actually, I've alread done a whole lot) to tear it apart and move the current semantics into the following new classes:
      NetworkServer - the actual network server code
      NetworkAdminServer - the implementation of network administration commands (ping, shutdown etc)
      NetworkAdminClient - the client for network administration of Derby
      NetworkAdminProtocol - the administration command protocol (as opposed to the DRDA protocol)
      plus a couple of utility classes (common methods, common constants, error messaging, exceptions etc)

      1. derby-2412-v1.stat
        1 kB
        Bernt M. Johnsen
      2. derby-2412-v1.diff
        293 kB
        Bernt M. Johnsen

        Issue Links

          Activity

          Hide
          Bernt M. Johnsen added a comment -

          There are still some bugs, but I will be on Gran Canaria for a week , so I upload the not quite finished version in case somebody wish to take a peek while I'm gone. Comments are most welcome.

          Show
          Bernt M. Johnsen added a comment - There are still some bugs, but I will be on Gran Canaria for a week , so I upload the not quite finished version in case somebody wish to take a peek while I'm gone. Comments are most welcome.
          Hide
          Daniel John Debrunner added a comment -

          bernt> Refactor ... bugs ...

          When I'm refactoring code and start to see bugs I get very nervous about my own changes, because the refactoring is only shuffling code around so it should be low risk for bugs. Even fixing the bugs that the tests find doesn't make me completely happy because what about the bugs I introduced that are not found by the tests.
          So I often take the approach of "throwing away all the work" and starting again (having now understood the direction I want to go), this time making small incremental steps and frequent commits/patches having run tests on each change. This gives me as the contributor greater confidence I'm not adding additional bugs and hopefully any reviewers greater confidence because the commit messages/patches are smaller and easier to understand and review.

          So may I propose (once you come back rested from what I hope is a vacation is Gran Canaria that you attack this issue in an incremental fashion. A 300k patch with bugs was not an easy thing to look at.

          Show
          Daniel John Debrunner added a comment - bernt> Refactor ... bugs ... When I'm refactoring code and start to see bugs I get very nervous about my own changes, because the refactoring is only shuffling code around so it should be low risk for bugs. Even fixing the bugs that the tests find doesn't make me completely happy because what about the bugs I introduced that are not found by the tests. So I often take the approach of "throwing away all the work" and starting again (having now understood the direction I want to go), this time making small incremental steps and frequent commits/patches having run tests on each change. This gives me as the contributor greater confidence I'm not adding additional bugs and hopefully any reviewers greater confidence because the commit messages/patches are smaller and easier to understand and review. So may I propose (once you come back rested from what I hope is a vacation is Gran Canaria that you attack this issue in an incremental fashion. A 300k patch with bugs was not an easy thing to look at.
          Hide
          Rick Hillegas added a comment -

          Thanks for tackling this refactoring, Bernt. I think that your proposed classes are a useful abstraction. They help to tease apart a lot of functionality which is all jumbled together today.

          I have a small comment about how the new classes handle command args. The old command arg handling was pretty brittle, I thought. It would be better if there were a CommandArg class which encapsulated all of the bits of a command arg, e.g., its user-visible name, how many parameters it requires, and the manifest constant it maps to and which is used by the switch statements. I think that this re-factoring has made the handling of command args a bit more brittle. Now the manifest constants live in one class (NetworkAdminProtocol) and the user-visible name and parameter counts live in another class (NetworkAdminClient).

          Show
          Rick Hillegas added a comment - Thanks for tackling this refactoring, Bernt. I think that your proposed classes are a useful abstraction. They help to tease apart a lot of functionality which is all jumbled together today. I have a small comment about how the new classes handle command args. The old command arg handling was pretty brittle, I thought. It would be better if there were a CommandArg class which encapsulated all of the bits of a command arg, e.g., its user-visible name, how many parameters it requires, and the manifest constant it maps to and which is used by the switch statements. I think that this re-factoring has made the handling of command args a bit more brittle. Now the manifest constants live in one class (NetworkAdminProtocol) and the user-visible name and parameter counts live in another class (NetworkAdminClient).
          Hide
          Bernt M. Johnsen added a comment -

          Dan: I see your point, and I will consider your approach, depending of the nature of the remaining problems. ( And, yes Gran Canaria was a vacation, I was not able to acces worm mail

          Rick: Good idea with a CommandArg clss, but that should be done as a separate iteration (ref Dan's comment).

          Show
          Bernt M. Johnsen added a comment - Dan: I see your point, and I will consider your approach, depending of the nature of the remaining problems. ( And, yes Gran Canaria was a vacation, I was not able to acces worm mail Rick: Good idea with a CommandArg clss, but that should be done as a separate iteration (ref Dan's comment).
          Hide
          Bernt M. Johnsen added a comment -

          I will back out this one. I think a more iterative approach like the one Dan proposed will work better. I will start afresh after 10.3 is out. I'll see if https://issues.apache.org/jira/browse/DERBY-2363 may be implemented for 10.3 without this one.

          Show
          Bernt M. Johnsen added a comment - I will back out this one. I think a more iterative approach like the one Dan proposed will work better. I will start afresh after 10.3 is out. I'll see if https://issues.apache.org/jira/browse/DERBY-2363 may be implemented for 10.3 without this one.
          Hide
          Myrna van Lunteren added a comment -

          I wonder, is this still being worked on?

          Show
          Myrna van Lunteren added a comment - I wonder, is this still being worked on?
          Hide
          Dag H. Wanvik added a comment -

          I don't believe so. Bernt is busy elsewhere these days.

          Show
          Dag H. Wanvik added a comment - I don't believe so. Bernt is busy elsewhere these days.
          Hide
          Dag H. Wanvik added a comment -

          Unassigning Bernt, so someone else can pick it up.

          Show
          Dag H. Wanvik added a comment - Unassigning Bernt, so someone else can pick it up.

            People

            • Assignee:
              Unassigned
              Reporter:
              Bernt M. Johnsen
            • Votes:
              2 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:

                Development