ctubbsii commented on PR #5425: URL: https://github.com/apache/accumulo/pull/5425#issuecomment-2749802759
> @ctubbsii #5358 added a Map of tables to balance in the balancerParams. That PR removed the need for single table constructors to be defined for balancers. I think my main confusion is that #5358 doesn't actually have a description of what it did. It just says it included a subset of changes from #5195. Except, #5195 also doesn't have a description of what it did either... it has an empty description. So, you have to dig into the code to see what is being changed at all... even if you've looked at it before, you can't rely on a description to refresh your memory, you have to look at the code and figure it out from scratch again. If either of those had a good description of what was actually being changed, I think I might have fewer questions here. > To avoid confusion, I looked into removing these constructors in 2.1 but the TableLoadBalancer requires all per-table balancer classes to use a single table constructor, even though that is not defined in the TabletBalancer spi interface. . It woudn't be documented in the SPI, because the TabletBalancer SPI wasn't written to be a per-table thing. That is/was specific to the features of the TableLoadBalancer implementation of the TabletBalancer interface. (The TableLoadBalancer impl would be better named "PerTableLoadBalancer" or "PerTableDelegatingLoadBalancer" to indicate the feature it is adding.) Granted, the TableLoadBalancer should have had a better javadoc to explain its delegating capabilities, and any requirements. I *think* you're saying that the new method(s) added to the SPI interface in #5358 made these constructors unnecessary. However, I don't understand how these new methods supersede or replace the per-table delegation that is part of the TableLoadBalancer impl. The requirements for per-table delegation in the TableLoadBalancer impl seem entirely separate from the requirements of the SPI, and I still see the constructors being called in the code in #5358, so it seems to continue to operate the same way it did before, as a delegating load balancer. So, it's not clear to me how the parameter passed to the constructor in that case is interacting with or is replaced by the new method(s). Did we add per-table load balancing capability at the top level? Is that what #5358 did? Or was that in another PR? > https://github.com/apache/accumulo/blob/e7a763b65359bb6d5caffc4c4f9be9ef0d0a473c/core/src/main/java/org/apache/accumulo/core/spi/balancer/TableLoadBalancer.java#L55-L59 > > I'm happy to repoint this PR to the 3.1 deprecation branch. The change to remove these balancer constructors would not happen until 4.0 anyway. It's fine if it gets deprecated here in 2.1 if there's actually a different means to satisfy the same use case... I'm just trying to understand if that's the case or not. Either way, we would have to include the deprecations in a 3.1 deprecation branch prior to 4.0, whether or not it gets merged into 2.1. Another hiccup for this is that the SPI added a new method that users are forced to implement for their own custom balancers, but it does not have a default method in the interface. So, it's going to break everybody's custom balancers. Is that something we want to do? Or should we provide a default implementation of TabletBalancer.getTablesToBalance that intersects params.getTablesToBalance with balancerEnv.getTableIdMap, or similar, so everybody's custom balancers aren't broken, especially in a patch release? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
