Derby
  1. Derby
  2. DERBY-4700

Add method to obtain a bogus port in TestConfiguration

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.7.1.1
    • Fix Version/s: 10.6.2.1, 10.7.1.1
    • Component/s: Test
    • Labels:
      None

      Description

      In some cases one needs to obtain a port number on which there is no Derby network server.
      Today this is solved in an ad-hoc way, for instance by using the current port minus one. When running tests in parallel, there is a chance that a port where there actually is a Derby network server listening is picked.

      As a start, I suggest that the bogus port is allocated to the last port in the port range configured for the run: baseport + MAX_PORTS_USED -1.

      1. derby-4700-2a-static_bogus.diff
        4 kB
        Kristian Waagan
      2. derby-4700-1b.diff
        5 kB
        Kristian Waagan
      3. derby-4700-1a.diff
        4 kB
        Kristian Waagan

        Issue Links

          Activity

          Kristian Waagan created issue -
          Hide
          Knut Anders Hatlen added a comment -

          Alternatively, we could initialize it using getNextAvailablePort() in TestConfiguration's constructor, similar to how we allocate the JMX port. Then we don't need extra logic in getNextAvailablePort() to prevent collisions with the last port in the allowed range.

          Show
          Knut Anders Hatlen added a comment - Alternatively, we could initialize it using getNextAvailablePort() in TestConfiguration's constructor, similar to how we allocate the JMX port. Then we don't need extra logic in getNextAvailablePort() to prevent collisions with the last port in the allowed range.
          Hide
          Kristian Waagan added a comment -

          Makes sense to follow an existing pattern. Note that there are eight constructors...
          BTW, the extra logic for the other approach would be changing '>' to '>='

          In a way we are wasting ports by allocating them even if they are not used, but I think it is better to allocate them early on for the sake of clarity.
          Should we also add debug print outs for the additional ports (jmx and bogus) to aid debugging port conflicts?

          Show
          Kristian Waagan added a comment - Makes sense to follow an existing pattern. Note that there are eight constructors... BTW, the extra logic for the other approach would be changing '>' to '>=' In a way we are wasting ports by allocating them even if they are not used, but I think it is better to allocate them early on for the sake of clarity. Should we also add debug print outs for the additional ports (jmx and bogus) to aid debugging port conflicts?
          Kristian Waagan made changes -
          Field Original Value New Value
          Assignee Kristian Waagan [ kristwaa ]
          Hide
          Kristian Waagan added a comment -

          First proposal for a patch, using the mechanism Knut mentioned above.

          Regression tests passed.
          I haven't looked through the existing tests yet for more cases where a bogus port is used.
          I'm also considering adding a println-statement where we assign a bogus port (i.e. bogusPort = getNextAvailablePort()).

          Show
          Kristian Waagan added a comment - First proposal for a patch, using the mechanism Knut mentioned above. Regression tests passed. I haven't looked through the existing tests yet for more cases where a bogus port is used. I'm also considering adding a println-statement where we assign a bogus port (i.e. bogusPort = getNextAvailablePort()).
          Kristian Waagan made changes -
          Attachment derby-4700-1a.diff [ 12448296 ]
          Kristian Waagan made changes -
          Issue & fix info [Patch Available]
          Hide
          Dag H. Wanvik added a comment -

          Looks good to me. +1 I have no special concerns about adding an println. The way I understand it, it is there to aid test debugging.

          Nit: I'd use the existing idiom for converting an int to String (in the changed line in NetworkServerControlClientCommandTest), since the same idiom is used elsewhere in that method.

          Show
          Dag H. Wanvik added a comment - Looks good to me. +1 I have no special concerns about adding an println. The way I understand it, it is there to aid test debugging. Nit: I'd use the existing idiom for converting an int to String (in the changed line in NetworkServerControlClientCommandTest), since the same idiom is used elsewhere in that method.
          Hide
          Kristian Waagan added a comment -

          Thanks, Dag.

          I have attached patch 1b, where I reverted to the idom used elsewhere (the "" + was just to shorten the line ), and I also added a println method in TestConfiguration (TC). Not quite sure about it yet, but it did at least made it clear to me that we are creating two TC instances - so we end up with two JMX ports and two bogus ports. I guess one is for the embedded configuration, the other for the client.

          Do we need a bogus port for an embedded TC?
          What about a JMX port?

          I'm a bit in a hurry right now, but will return to this later.

          Show
          Kristian Waagan added a comment - Thanks, Dag. I have attached patch 1b, where I reverted to the idom used elsewhere (the "" + was just to shorten the line ), and I also added a println method in TestConfiguration (TC). Not quite sure about it yet, but it did at least made it clear to me that we are creating two TC instances - so we end up with two JMX ports and two bogus ports. I guess one is for the embedded configuration, the other for the client. Do we need a bogus port for an embedded TC? What about a JMX port? I'm a bit in a hurry right now, but will return to this later.
          Kristian Waagan made changes -
          Attachment derby-4700-1b.diff [ 12448484 ]
          Hide
          Kristian Waagan added a comment -

          A comment from Dag, posted to derby-dev:


          > Do we need a bogus port for an embedded TC?
          Well, I used one in the BootLockTest, for example (although Kathey has
          later rewritten that test to work without ports), so I wouldn't rule
          it a priori. I wasn't aware that we created two concurrent TCs either,
          presumably they are used in sequence, so we don't really need separate
          port assignements for them?

          > What about a JMX port?
          I think JMX is useful also for an embedded Derby, so we would want to
          be able to test it.

          Thanks,
          Dag


          Show
          Kristian Waagan added a comment - A comment from Dag, posted to derby-dev: > Do we need a bogus port for an embedded TC? Well, I used one in the BootLockTest, for example (although Kathey has later rewritten that test to work without ports), so I wouldn't rule it a priori. I wasn't aware that we created two concurrent TCs either, presumably they are used in sequence, so we don't really need separate port assignements for them? > What about a JMX port? I think JMX is useful also for an embedded Derby, so we would want to be able to test it. Thanks, Dag
          Hide
          Kristian Waagan added a comment -

          Committed patch 1a to trunk with revision 963705.
          Backported to 10.6 with revision 963706.

          I don't expect more work on this issue.

          Show
          Kristian Waagan added a comment - Committed patch 1a to trunk with revision 963705. Backported to 10.6 with revision 963706. I don't expect more work on this issue.
          Kristian Waagan made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Issue & fix info [Patch Available]
          Fix Version/s 10.6.1.1 [ 12314973 ]
          Fix Version/s 10.7.0.0 [ 12314971 ]
          Resolution Fixed [ 1 ]
          Hide
          Kristian Waagan added a comment -

          Patch 2a makes the bogus port static, such that it is shared between all TestConfiguration instances in a VM. It also increases the max port count to 11 - we're already using that many ports so the next change that requires another alternative port will trigger the assert in getNextAvailablePort().

          Regression tests passed (traditional serial run).
          Patch ready for review.
          I do plan to commit this fix shortly to stop the tinderbox test from failing. I'll update the wiki page when I commit.

          Show
          Kristian Waagan added a comment - Patch 2a makes the bogus port static, such that it is shared between all TestConfiguration instances in a VM. It also increases the max port count to 11 - we're already using that many ports so the next change that requires another alternative port will trigger the assert in getNextAvailablePort(). Regression tests passed (traditional serial run). Patch ready for review. I do plan to commit this fix shortly to stop the tinderbox test from failing. I'll update the wiki page when I commit.
          Kristian Waagan made changes -
          Attachment derby-4700-2a-static_bogus.diff [ 12449448 ]
          Kristian Waagan made changes -
          Issue & fix info [Patch Available]
          Hide
          Kristian Waagan added a comment -

          To stop the tinderbox from failing, I decided to commit patch 2a.
          I increased the maximum number of ports to 20, and also updated the wiki.
          This can be changed again if the discussion on derby-dev ends with a different conclusion.

          Committed to trunk with revision 964115, I will backport it to 10.6 tomorrow if there are no problems.

          Show
          Kristian Waagan added a comment - To stop the tinderbox from failing, I decided to commit patch 2a. I increased the maximum number of ports to 20, and also updated the wiki. This can be changed again if the discussion on derby-dev ends with a different conclusion. Committed to trunk with revision 964115, I will backport it to 10.6 tomorrow if there are no problems.
          Kristian Waagan made changes -
          Issue & fix info [Patch Available]
          Tiago R. Espinha made changes -
          Link This issue is related to DERBY-4747 [ DERBY-4747 ]
          Hide
          Kristian Waagan added a comment -

          Backported patch 2a to 10.6 with revision 965737.
          Closing issue.

          Show
          Kristian Waagan added a comment - Backported patch 2a to 10.6 with revision 965737. Closing issue.
          Kristian Waagan made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Kathey Marsden made changes -
          Fix Version/s 10.6.2.0 [ 12315342 ]
          Fix Version/s 10.6.1.1 [ 12314973 ]
          Knut Anders Hatlen made changes -
          Fix Version/s 10.6.2.1 [ 12315343 ]
          Fix Version/s 10.6.2.0 [ 12315342 ]
          Rick Hillegas made changes -
          Affects Version/s 10.7.1.1 [ 12315564 ]
          Affects Version/s 10.7.1.0 [ 12314971 ]
          Fix Version/s 10.7.1.1 [ 12315564 ]
          Fix Version/s 10.7.1.0 [ 12314971 ]
          Gavin made changes -
          Workflow jira [ 12513372 ] Default workflow, editable Closed status [ 12799740 ]

            People

            • Assignee:
              Kristian Waagan
              Reporter:
              Kristian Waagan
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development