Solr
  1. Solr
  2. SOLR-959

Remove hardcoded port numbers from TestReplicationHandler

    Details

    • Type: Test Test
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.4
    • Component/s: replication (java)
    • Labels:
      None

      Description

      TestReplicationHandler has a hardcoded port of 9999 in it for the "master". hardcoding port numbers in unit tests is very brittle and error prone and can cause problems. Ideally tests that aren't explicitly testing network related functionality should avoid dealing with the network at all, but when neccessary it's much better to let the OS pick any available port (as most other solr tests do) then to hardcoded it.

      in TestReplicationHandler things are a little more complicated because the master port number needs to be refered to in the slave config files.

      1. replicationtest-port-refactor.patch
        6 kB
        Hoss Man
      2. SOLR-959.patch
        12 kB
        Akshay K. Ukey

        Activity

        Hide
        Hoss Man added a comment -

        first pass at a refactoring to clean up the hardcoded ports – takes advantage of the copyFile function that was already in the test to replace a marker token in the config with the real port number.

        at first glance this refactoring should work, but testIndexAndConfigAliasReplication errors with this patch --i believe the problem is because it expects the slave to pull files from a master after the master server has been stoped and then restarted – which fails because the new master won't have the same port number. we could try explicitly creating the new master with the same port as the old master, but there's no guarantee the os won't have already allocated that port to a new process at that point. A better solution is to eliminate the need to stop/start the master by using core reload or something similar instead.

        The most alarming thing at this point is that several other test methods do similar stop/start of the master, and yet they work fine (even though the slave has no knowledge of the new master port) which suggests to me that the tests may be false positives making flawed assumptions.

        unfortunately i don't have any more time to look into this at the moment ... attaching patch as a checkpoint in case someone else wants to look into it (or i get hit by a bus)

        Show
        Hoss Man added a comment - first pass at a refactoring to clean up the hardcoded ports – takes advantage of the copyFile function that was already in the test to replace a marker token in the config with the real port number. at first glance this refactoring should work, but testIndexAndConfigAliasReplication errors with this patch --i believe the problem is because it expects the slave to pull files from a master after the master server has been stoped and then restarted – which fails because the new master won't have the same port number. we could try explicitly creating the new master with the same port as the old master, but there's no guarantee the os won't have already allocated that port to a new process at that point. A better solution is to eliminate the need to stop/start the master by using core reload or something similar instead. The most alarming thing at this point is that several other test methods do similar stop/start of the master, and yet they work fine (even though the slave has no knowledge of the new master port) which suggests to me that the tests may be false positives making flawed assumptions. unfortunately i don't have any more time to look into this at the moment ... attaching patch as a checkpoint in case someone else wants to look into it (or i get hit by a bus)
        Hide
        Akshay K. Ukey added a comment -

        Patch with re-factoring from previous patch and few more changes.

        Show
        Akshay K. Ukey added a comment - Patch with re-factoring from previous patch and few more changes.
        Hide
        Shalin Shekhar Mangar added a comment -

        Thanks Hoss and Akshay.

        The most alarming thing at this point is that several other test methods do similar stop/start of the master, and yet they work fine (even though the slave has no knowledge of the new master port) which suggests to me that the tests may be false positives making flawed assumptions.

        Hoss, perhaps the master's port did not change for these tests? It would be best to stop the slaves and re-create their solrconfig using the new master instance's port which is what Akshay has done in the latest patch.

        The latest patch looks good, works well. I'll commit this shortly.

        Show
        Shalin Shekhar Mangar added a comment - Thanks Hoss and Akshay. The most alarming thing at this point is that several other test methods do similar stop/start of the master, and yet they work fine (even though the slave has no knowledge of the new master port) which suggests to me that the tests may be false positives making flawed assumptions. Hoss, perhaps the master's port did not change for these tests? It would be best to stop the slaves and re-create their solrconfig using the new master instance's port which is what Akshay has done in the latest patch. The latest patch looks good, works well. I'll commit this shortly.
        Hide
        Shalin Shekhar Mangar added a comment -

        Committed revision 741684.

        Thanks Hoss and Akshay!

        Show
        Shalin Shekhar Mangar added a comment - Committed revision 741684. Thanks Hoss and Akshay!
        Hide
        Grant Ingersoll added a comment -

        Bulk close for Solr 1.4

        Show
        Grant Ingersoll added a comment - Bulk close for Solr 1.4

          People

          • Assignee:
            Shalin Shekhar Mangar
            Reporter:
            Hoss Man
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development