Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.99.0
    • Fix Version/s: hbase-10070
    • Component/s: master
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      LoadBalancer has to be aware of and enforce placement of region replicas so that the replicas are not co-hosted in the same server, host or rack. This will ensure that the region is highly available during process / host / rack failover.

      1. hbase-10351_v7.patch
        116 kB
        Enis Soztutar
      2. hbase-10351_v6.patch
        121 kB
        Enis Soztutar
      3. hbase-10351_v5.patch
        121 kB
        Enis Soztutar
      4. hbase-10351_v3.patch
        115 kB
        Enis Soztutar
      5. hbase-10351_v1.patch
        110 kB
        Enis Soztutar
      6. hbase-10351_v0.patch
        101 kB
        Enis Soztutar

        Issue Links

          Activity

          Hide
          Enis Soztutar added a comment -

          Review board is down again. Attaching v0 patch here instead.

          This patch builds on top of HBASE-10350, and includes changes from Devaraj Das and myself for load balancer to enforce region replica placement.

          In short, HBASE-10350 enables table creation with region replicas, but won't have any region placement enforcement. This patch, adds co-location constraints to the LB, so that it does a best effort job to not place replicas of the same region in same hosts / racks.

          The overview of changes are:

          • BaseLoadBalancer.Cluster is aware of hosts and racks for servers
          • BaseLoadBalancer.Cluster is aware of regions per host / rack
          • BaseLoadBalancer.Cluster keeps track of region replicas for co-location enforcement
          • BaseLoadBalancer.Cluster can be constructed with unassigned regions as well. Some refactoring (Action, etc) for better abstractions etc.
          • BaseLoadBalancer.retainAssignments(), etc now construct a Cluster object and use the methods there to ensure co-location constraint. This is a first step in unifying the way we do balance() and table creation, retainAssignment() etc. We can continue with this in other jiras (not part of HBASE-10070).
          • StochasticLoadBalancer has (high) costs for host/rack replica co-locations and candidate generator for ensuring optimum plan generation. By the way of these cost functions, the LB can satisfy that replicas are not co-located as long as there are enough servers / rack left. If not, it will still do assignment (soft constraint)
          • Bunch of tests to ensure region placement (and speed of balance() run)
          Show
          Enis Soztutar added a comment - Review board is down again. Attaching v0 patch here instead. This patch builds on top of HBASE-10350 , and includes changes from Devaraj Das and myself for load balancer to enforce region replica placement. In short, HBASE-10350 enables table creation with region replicas, but won't have any region placement enforcement. This patch, adds co-location constraints to the LB, so that it does a best effort job to not place replicas of the same region in same hosts / racks. The overview of changes are: BaseLoadBalancer.Cluster is aware of hosts and racks for servers BaseLoadBalancer.Cluster is aware of regions per host / rack BaseLoadBalancer.Cluster keeps track of region replicas for co-location enforcement BaseLoadBalancer.Cluster can be constructed with unassigned regions as well. Some refactoring (Action, etc) for better abstractions etc. BaseLoadBalancer.retainAssignments(), etc now construct a Cluster object and use the methods there to ensure co-location constraint. This is a first step in unifying the way we do balance() and table creation, retainAssignment() etc. We can continue with this in other jiras (not part of HBASE-10070 ). StochasticLoadBalancer has (high) costs for host/rack replica co-locations and candidate generator for ensuring optimum plan generation. By the way of these cost functions, the LB can satisfy that replicas are not co-located as long as there are enough servers / rack left. If not, it will still do assignment (soft constraint) Bunch of tests to ensure region placement (and speed of balance() run)
          Hide
          Enis Soztutar added a comment -

          updated patch for changes in upstream patches (HBASE-10350, HBASE-10361)

          Show
          Enis Soztutar added a comment - updated patch for changes in upstream patches ( HBASE-10350 , HBASE-10361 )
          Hide
          Enis Soztutar added a comment -

          updated patch for working on top of HBASE-10350. I've fixed a couple of issues with retainAssignment() and roundRobinAssignment()

          Show
          Enis Soztutar added a comment - updated patch for working on top of HBASE-10350 . I've fixed a couple of issues with retainAssignment() and roundRobinAssignment()
          Hide
          Enis Soztutar added a comment -

          I could not get RB to apply my patch (with a parent patch).

          You can review from github:
          https://github.com/enis/hbase/commit/276f8256a7241247d2fc9f7c02adf28f3fbd9ba5

          Show
          Enis Soztutar added a comment - I could not get RB to apply my patch (with a parent patch). You can review from github: https://github.com/enis/hbase/commit/276f8256a7241247d2fc9f7c02adf28f3fbd9ba5
          Hide
          Sergey Shelukhin added a comment -

          I am reviewing, btw

          Show
          Sergey Shelukhin added a comment - I am reviewing, btw
          Hide
          Sergey Shelukhin added a comment -

          Left some comments yesterday and today... I didn't review tests very carefully. The rest seems ok (except where noted) although the array manipulations are rather complex and meaning is not always clear, so I might have missed something. In general more comments would help

          Show
          Sergey Shelukhin added a comment - Left some comments yesterday and today... I didn't review tests very carefully. The rest seems ok (except where noted) although the array manipulations are rather complex and meaning is not always clear, so I might have missed something. In general more comments would help
          Hide
          Devaraj Das added a comment -

          Other than the point Sergey raised about putting some comments in the code around the array manipulations, and maybe breaking the manipulations into smaller methods, it looks good to me.
          The one issue that is there I think is that the test (TestMasterOperationsForReplicas) may be flaky (the part where retain assignments is set to true). The reason being, that in the retainAssignment there is max iterations for choosing the server to place the region in but that may not be correct location given the replicas. I think it's fine to leave the balancer's assign methods as you currently have it (eventually the balancer.balance will fix it), but modify the test to be able to handle that.
          Another minor nit is that in roundRobinAssignment, we could land up in the catch-all anytime we have numReplicas > numRegionServers. The comment in the catch-all code should be updated.
          The refactor to do with introducing the Action class is good!

          Show
          Devaraj Das added a comment - Other than the point Sergey raised about putting some comments in the code around the array manipulations, and maybe breaking the manipulations into smaller methods, it looks good to me. The one issue that is there I think is that the test (TestMasterOperationsForReplicas) may be flaky (the part where retain assignments is set to true). The reason being, that in the retainAssignment there is max iterations for choosing the server to place the region in but that may not be correct location given the replicas. I think it's fine to leave the balancer's assign methods as you currently have it (eventually the balancer.balance will fix it), but modify the test to be able to handle that. Another minor nit is that in roundRobinAssignment, we could land up in the catch-all anytime we have numReplicas > numRegionServers. The comment in the catch-all code should be updated. The refactor to do with introducing the Action class is good!
          Hide
          Sergey Shelukhin added a comment -

          Is this the same patch I reviewed on github? What is the refactor of Action class, I may have missed that

          Show
          Sergey Shelukhin added a comment - Is this the same patch I reviewed on github? What is the refactor of Action class, I may have missed that
          Hide
          Sergey Shelukhin added a comment -

          oh, nm, I thought it's about MultiAction-Action part, the cluster actions I have seen

          Show
          Sergey Shelukhin added a comment - oh, nm, I thought it's about MultiAction-Action part, the cluster actions I have seen
          Hide
          Enis Soztutar added a comment -

          Left some comments yesterday and today

          Thanks! v4 patch addressed all the reviews there.

          although the array manipulations are rather complex and meaning is not always clear

          When we unwound hashmap + hashset operations into array and index based ones for performance, we ended up un-javaifying that code. Not much to do about it (see HBASE-8119 for ref)

          The one issue that is there I think is that the test (TestMasterOperationsForReplicas) may be flaky

          It seems fine in my runs. retain assignment now also looks for availability and finds the best server while retaining the host locality. We can fix it if we see flakiness.

          The comment in the catch-all code should be updated.

          Fixed.

          Show
          Enis Soztutar added a comment - Left some comments yesterday and today Thanks! v4 patch addressed all the reviews there. although the array manipulations are rather complex and meaning is not always clear When we unwound hashmap + hashset operations into array and index based ones for performance, we ended up un-javaifying that code. Not much to do about it (see HBASE-8119 for ref) The one issue that is there I think is that the test (TestMasterOperationsForReplicas) may be flaky It seems fine in my runs. retain assignment now also looks for availability and finds the best server while retaining the host locality. We can fix it if we see flakiness. The comment in the catch-all code should be updated. Fixed.
          Show
          Enis Soztutar added a comment - Thanks for reviews! v4 changes: https://github.com/enis/hbase/commit/f32d581d8b832905d9aa5753f2fd09bd83a1fe5e v5 changes: https://github.com/enis/hbase/commit/89dd40875af96e74405a727f32a2192afc8bfb8e Complete v5 patch: https://github.com/enis/hbase/compare/55355004fe21e926f7d0053b9aae154c742fbd45...hbase-10351#diff-ee791396bd76597ef9f2ecddc4c6d112L63 I am also attaching the v5 patch.
          Hide
          Sergey Shelukhin added a comment -

          some minor comments (in response to old comments and on v4). Mostly looks good

          Show
          Sergey Shelukhin added a comment - some minor comments (in response to old comments and on v4). Mostly looks good
          Hide
          Enis Soztutar added a comment -
          Show
          Enis Soztutar added a comment - Thanks Sergey. I've addressed the comments in v6: https://github.com/enis/hbase/commit/0743a551dba2e6d3428ea3064929b7ff46734c9f
          Hide
          Sergey Shelukhin added a comment -

          +1

          Show
          Sergey Shelukhin added a comment - +1
          Hide
          Enis Soztutar added a comment -

          Opened HBASE-10620 for fixing LB.needsBalance() for region replicas.

          Show
          Enis Soztutar added a comment - Opened HBASE-10620 for fixing LB.needsBalance() for region replicas.
          Hide
          Enis Soztutar added a comment -

          I've committed this to branch. Thanks Devaraj and Sergey for reviews.

          Show
          Enis Soztutar added a comment - I've committed this to branch. Thanks Devaraj and Sergey for reviews.
          Hide
          Enis Soztutar added a comment -

          v7 is the committed version with minor conflict resolution.

          Show
          Enis Soztutar added a comment - v7 is the committed version with minor conflict resolution.

            People

            • Assignee:
              Enis Soztutar
              Reporter:
              Enis Soztutar
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development