Uploaded image for project: 'Accumulo'
  1. Accumulo
  2. ACCUMULO-4549

Remove duplicate init functions in TabletBalancer

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.7.3, 1.8.1, 2.0.0
    • Component/s: None
    • Labels:

      Description

      There are two init functions with similar signatures in the TabletLoadBalancer class that could result in an end-user overriding the wrong function and their object may never be initialized.

      Master (1) calls the TabletBalancer#init(ServerConfigurationFactory) which is not what the regex load balancer is overriding (2), and the TabletBalancer doesn't call the other init function (3).

      Come up with a plan for compatibility and remove the duplicate functions.

      (1) https://github.com/apache/accumulo/blob/1.7/server/master/src/main/java/org/apache/accumulo/master/Master.java#L589
      (2) https://github.com/apache/accumulo/blob/1.7/server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java#L257
      (3) https://github.com/apache/accumulo/blob/1.7/server/base/src/main/java/org/apache/accumulo/server/master/balancer/TabletBalancer.java#L57

        Issue Links

          Activity

          Hide
          ctubbsii Christopher Tubbs added a comment -

          I briefly looked into this. It's actually kinda frustrating, because the ServerConfigurationFactory really shouldn't be the base object used here. As an interim solution, we can deprecate the old method with ServerConfiguration but it really should be injecting the Master context, or a dedicated configuration object available for balancers. If we're going to treat the TabletBalancer as API, we're going to need to ensure we treat any method parameter types as public API, too, and that's way out of scope for ServerConfigurationFactory.

          It looks like the main reason this is even needed in the TabletBalancer is to construct a context, which carries the RPC credentials, so the balancer can do thrift calls to get online tablets lists. It seems to me that this isn't really necessary, because the Master should have that information. However, it's also used to get TableConfiguration instances in some subclasses.

          In short, we can deprecate one of them, but this is a really terrible API, with tons of scope creep on these server configuration objects, which will have to be improved significantly before promoting the balancer to "public API".

          Show
          ctubbsii Christopher Tubbs added a comment - I briefly looked into this. It's actually kinda frustrating, because the ServerConfigurationFactory really shouldn't be the base object used here. As an interim solution, we can deprecate the old method with ServerConfiguration but it really should be injecting the Master context, or a dedicated configuration object available for balancers. If we're going to treat the TabletBalancer as API, we're going to need to ensure we treat any method parameter types as public API, too, and that's way out of scope for ServerConfigurationFactory . It looks like the main reason this is even needed in the TabletBalancer is to construct a context, which carries the RPC credentials, so the balancer can do thrift calls to get online tablets lists. It seems to me that this isn't really necessary, because the Master should have that information. However, it's also used to get TableConfiguration instances in some subclasses. In short, we can deprecate one of them, but this is a really terrible API, with tons of scope creep on these server configuration objects, which will have to be improved significantly before promoting the balancer to "public API".
          Hide
          ctubbsii Christopher Tubbs added a comment -

          I'm deprecating in 1.7.3 and 1.8.1, and removing in 2.0.0/master branch.

          Show
          ctubbsii Christopher Tubbs added a comment - I'm deprecating in 1.7.3 and 1.8.1, and removing in 2.0.0/master branch.

            People

            • Assignee:
              ctubbsii Christopher Tubbs
              Reporter:
              adamjshook Adam J Shook
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0h
                0h
                Logged:
                Time Spent - 0.5h
                0.5h

                  Development