Solr
  1. Solr
  2. SOLR-6485

ReplicationHandler should have an option to throttle the speed of replication

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.0, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      The ReplicationHandler should have an option to throttle the speed of replication.

      It is useful for people who want bring up nodes in their SolrCloud cluster or when have a backup-restore API and not eat up all their network bandwidth while replicating.

      I am writing a test case and will attach a patch shortly.

      1. SOLR-6485.patch
        22 kB
        Varun Thacker
      2. SOLR-6485.patch
        22 kB
        Noble Paul
      3. SOLR-6485.patch
        20 kB
        Varun Thacker
      4. SOLR-6485.patch
        19 kB
        Varun Thacker
      5. SOLR-6485.patch
        17 kB
        Varun Thacker
      6. SOLR-6485.patch
        15 kB
        Varun Thacker
      7. SOLR-6485.patch
        11 kB
        Varun Thacker

        Issue Links

          Activity

          Hide
          Varun Thacker added a comment -

          Patch which adds throttling to the ReplicationHandler along with a test

          Show
          Varun Thacker added a comment - Patch which adds throttling to the ReplicationHandler along with a test
          Hide
          Varun Thacker added a comment -

          New patch. The previous approach was wrong. Now I am using SimpleRateLimiter.

          Debugging as to why it's not throttling correctly.

          Show
          Varun Thacker added a comment - New patch. The previous approach was wrong. Now I am using SimpleRateLimiter. Debugging as to why it's not throttling correctly.
          Hide
          Noble Paul added a comment -

          Any reason why you could not reuse the DirectoryFileInputStream class? I see a lot of code being duplicated

          Show
          Noble Paul added a comment - Any reason why you could not reuse the DirectoryFileInputStream class? I see a lot of code being duplicated
          Hide
          Mark Miller added a comment -

          Good issue overall. I have not looked so closely, but I had the same initial worry about some code duplication.

          Minor cleanup nits: rateLimiiter variable is misspelled, there is a System.out left in.

          Show
          Mark Miller added a comment - Good issue overall. I have not looked so closely, but I had the same initial worry about some code duplication. Minor cleanup nits: rateLimiiter variable is misspelled, there is a System.out left in.
          Hide
          Ramkumar Aiyengar added a comment -

          Doesn't commit reserve duration have to account for the time taken by the pauses? As it stands currently, you run the risk of replication failing if ypu are throttling fairly highly.

          Show
          Ramkumar Aiyengar added a comment - Doesn't commit reserve duration have to account for the time taken by the pauses? As it stands currently, you run the risk of replication failing if ypu are throttling fairly highly.
          Hide
          Varun Thacker added a comment -

          Thanks everyone for reviewing!

          New patch which does the following -

          • Refactored the DirectoryFileStream and LocalFsFileStream to reuse code. The throttling logic is folded in DirectoryFileStream instead of having a separate implementation
          • The init pause is needed for the future pause timings to work correctly. Check TestRateLimiter.testPause() which does the same thing. Test fails when removing the init in testPause() method. We should fix this in another Jira I guess.
          • Test case and some small refactoring in TestReplicationHandler

          About reserving index commit points, currently we extend it by 10s for every 5 reads of 1MB ( default ) packet sizes. If one throttles a lot this could trip like Ramkumar pointed out.

          So we could take two approaches -
          1. Instead of the hardcoded 10 seconds look at the throttle speed and estimate how much time should the reserve point be extended for.
          2. Add functionality to the RateLimiter that actually returns the time it needs to pause before actually pausing. We could split the current pause method into 3 methods -
          preparePause()- would return the pauseNS value.
          pause(int pauseNS) - Pause for the specified time duration.
          pause() - calls preparePause() and then pause(pauseNs) . This makes sure that we are not breaking the public api.

          We could then take the time it needs to pause for and add it to the default 10 seconds.

          Show
          Varun Thacker added a comment - Thanks everyone for reviewing! New patch which does the following - Refactored the DirectoryFileStream and LocalFsFileStream to reuse code. The throttling logic is folded in DirectoryFileStream instead of having a separate implementation The init pause is needed for the future pause timings to work correctly. Check TestRateLimiter.testPause() which does the same thing. Test fails when removing the init in testPause() method. We should fix this in another Jira I guess. Test case and some small refactoring in TestReplicationHandler About reserving index commit points, currently we extend it by 10s for every 5 reads of 1MB ( default ) packet sizes. If one throttles a lot this could trip like Ramkumar pointed out. So we could take two approaches - 1. Instead of the hardcoded 10 seconds look at the throttle speed and estimate how much time should the reserve point be extended for. 2. Add functionality to the RateLimiter that actually returns the time it needs to pause before actually pausing. We could split the current pause method into 3 methods - preparePause()- would return the pauseNS value. pause(int pauseNS) - Pause for the specified time duration. pause() - calls preparePause() and then pause(pauseNs) . This makes sure that we are not breaking the public api. We could then take the time it needs to pause for and add it to the default 10 seconds.
          Hide
          Ramkumar Aiyengar added a comment -

          Could rate limiter expose a 'dry-run' version of pause, I.e. something which tells you how much it would pause if pause(N) was done? I think it's still cleaner to call pause(N), but an additional checkPauseTime(N) would give us an idea of how much to reserve for..

          Show
          Ramkumar Aiyengar added a comment - Could rate limiter expose a 'dry-run' version of pause, I.e. something which tells you how much it would pause if pause(N) was done? I think it's still cleaner to call pause(N), but an additional checkPauseTime(N) would give us an idea of how much to reserve for..
          Hide
          Noble Paul added a comment -

          Ramkumar Aiyengar I didn't quite understand the 'dry run' thing, I guess it should be taken up as a separate issue instead of mixing with this

          Show
          Noble Paul added a comment - Ramkumar Aiyengar I didn't quite understand the 'dry run' thing, I guess it should be taken up as a separate issue instead of mixing with this
          Hide
          Ramkumar Aiyengar added a comment -

          I was responding to Varun's two proposals for fixing the issue with commit reservations the current patch for this ticket has. In particular, I was suggesting a variant of his second proposal.

          Show
          Ramkumar Aiyengar added a comment - I was responding to Varun's two proposals for fixing the issue with commit reservations the current patch for this ticket has. In particular, I was suggesting a variant of his second proposal.
          Hide
          Varun Thacker added a comment -

          Well I think the best approach would be to use saveCommitPoint() and releaseCommitPoint() methods. Running tests now.

          Also I created LUCENE-5948 to fix the init issue in the RateLimiter.

          Show
          Varun Thacker added a comment - Well I think the best approach would be to use saveCommitPoint() and releaseCommitPoint() methods. Running tests now. Also I created LUCENE-5948 to fix the init issue in the RateLimiter.
          Hide
          Varun Thacker added a comment -

          New patch:

          • Removes a couple of unused variables
          • Uses saveCommitPoint() and releaseCommitPoint() methods instead of the time duration methods.
          Show
          Varun Thacker added a comment - New patch: Removes a couple of unused variables Uses saveCommitPoint() and releaseCommitPoint() methods instead of the time duration methods.
          Hide
          Ramkumar Aiyengar added a comment - - edited

          You still need the 10s commit reservation. The reason being, replication happens like this:

          • Replica gets list of files from leader
          • For each file, replica fetches the file from leader

          After the first step happens, you need to keep the commit alive for the entire duration of the second step which involves many, many requests. So you need to keep extending the commit reservation between requests. I think the save/release is clean for the duration of the write, you probably still need the older release at the end of it..

          Show
          Ramkumar Aiyengar added a comment - - edited You still need the 10s commit reservation. The reason being, replication happens like this: Replica gets list of files from leader For each file, replica fetches the file from leader After the first step happens, you need to keep the commit alive for the entire duration of the second step which involves many, many requests. So you need to keep extending the commit reservation between requests. I think the save/release is clean for the duration of the write, you probably still need the older release at the end of it..
          Hide
          Noble Paul added a comment -

          Yeah ,
          there is a need to save the commit point between two requests . I don't see a reason to make it configurable though.

          So , after the file is completely written out , reserve it for some time say 10 secs

          Show
          Noble Paul added a comment - Yeah , there is a need to save the commit point between two requests . I don't see a reason to make it configurable though. So , after the file is completely written out , reserve it for some time say 10 secs
          Hide
          Varun Thacker added a comment -

          You still need the 10s commit reservation. The reason being, replication happens like this:
          Replica gets list of files from leader
          For each file, replica fetches the file from leader

          Right nice catch!
          So this is what the new patch does -
          1. Saves the commit point for the duration of the write. We will not loose the commit point no matter how much the user throttles.
          2. At the end of the write it extends it by 10s so that the commit point is reserved across file fetches
          3. Added an extra condition to the test case - I add extra docs to the master after I call replicate. These docs should not be reflected in the slave server.

          Show
          Varun Thacker added a comment - You still need the 10s commit reservation. The reason being, replication happens like this: Replica gets list of files from leader For each file, replica fetches the file from leader Right nice catch! So this is what the new patch does - 1. Saves the commit point for the duration of the write. We will not loose the commit point no matter how much the user throttles. 2. At the end of the write it extends it by 10s so that the commit point is reserved across file fetches 3. Added an extra condition to the test case - I add extra docs to the master after I call replicate. These docs should not be reflected in the slave server.
          Hide
          Ramkumar Aiyengar added a comment -

          Looks about right. I haven't checked fully, but shouldn't the reserve and release in finally be swapped? Otherwise you have a race..

          Show
          Ramkumar Aiyengar added a comment - Looks about right. I haven't checked fully, but shouldn't the reserve and release in finally be swapped? Otherwise you have a race..
          Hide
          Noble Paul added a comment -

          added documentation and refactored a bit

          Show
          Noble Paul added a comment - added documentation and refactored a bit
          Hide
          Varun Thacker added a comment -

          Builds on Noble's patch. 3 small changes -
          1. Removes the initial pause needed in the DirectoryFileStream ctor
          2. Does a searcher decref() in the finally block like it originally did. I should not have removed it in the first place
          3. Changes the RateLimiter class and initializes the lastNS variable. This is needed for the test to pass.

          We should commit this only after LUCENE-5948 is committed.

          Show
          Varun Thacker added a comment - Builds on Noble's patch. 3 small changes - 1. Removes the initial pause needed in the DirectoryFileStream ctor 2. Does a searcher decref() in the finally block like it originally did. I should not have removed it in the first place 3. Changes the RateLimiter class and initializes the lastNS variable. This is needed for the test to pass. We should commit this only after LUCENE-5948 is committed.
          Hide
          ASF subversion and git services added a comment -

          Commit 1627340 from Noble Paul in branch 'dev/trunk'
          [ https://svn.apache.org/r1627340 ]

          SOLR-6485

          Show
          ASF subversion and git services added a comment - Commit 1627340 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1627340 ] SOLR-6485
          Hide
          ASF subversion and git services added a comment -

          Commit 1627341 from Noble Paul in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1627341 ]

          SOLR-6485

          Show
          ASF subversion and git services added a comment - Commit 1627341 from Noble Paul in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1627341 ] SOLR-6485
          Hide
          ASF subversion and git services added a comment -

          Commit 1631426 from Noble Paul in branch 'dev/trunk'
          [ https://svn.apache.org/r1631426 ]

          SOLR-6485 added javadocs

          Show
          ASF subversion and git services added a comment - Commit 1631426 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1631426 ] SOLR-6485 added javadocs
          Hide
          ASF subversion and git services added a comment -

          Commit 1631429 from Noble Paul in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1631429 ]

          SOLR-6485 added javadocs

          Show
          ASF subversion and git services added a comment - Commit 1631429 from Noble Paul in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1631429 ] SOLR-6485 added javadocs
          Hide
          ASF subversion and git services added a comment -

          Commit 1631554 from hossman@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1631554 ]

          SOLR-6485: remove useless javadocs that are broken and violate precommit

          Show
          ASF subversion and git services added a comment - Commit 1631554 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1631554 ] SOLR-6485 : remove useless javadocs that are broken and violate precommit
          Hide
          ASF subversion and git services added a comment -

          Commit 1631559 from hossman@apache.org in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1631559 ]

          SOLR-6485: remove useless javadocs that are broken and violate precommit (merge r1631554)

          Show
          ASF subversion and git services added a comment - Commit 1631559 from hossman@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1631559 ] SOLR-6485 : remove useless javadocs that are broken and violate precommit (merge r1631554)
          Hide
          Anshum Gupta added a comment -

          Bulk close after 5.0 release.

          Show
          Anshum Gupta added a comment - Bulk close after 5.0 release.

            People

            • Assignee:
              Noble Paul
              Reporter:
              Varun Thacker
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development