Fix ChaoticBalancerIT #401
Conversation
| @@ -42,6 +42,7 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg, Configuration hadoo | |||
| Map<String,String> siteConfig = cfg.getSiteConfig(); | |||
| siteConfig.put(Property.TSERV_MAXMEM.getKey(), "10K"); | |||
| siteConfig.put(Property.TSERV_MAJC_DELAY.getKey(), "0"); | |||
| siteConfig.put(Property.TABLE_LOAD_BALANCER.getKey(), ChaoticLoadBalancer.class.getName()); | |||
There was a problem hiding this comment.
I think the idea of this test was not just to test the ChaoticBalancer, but also to test the per-table balancer configuration feature. What you've done here should work, but it's probably best to set the property at table creation time using NewTableConfiguration rather than move it to the site configuration. That way, we can still test that per-table configuration path for having a custom balancer for a specific table and you still have the property set before the table is brought online for the first time.
(Also, I think it probably doesn't matter... but I'm not sure what the impact of setting this balancer for all tables will have. We may take precautions so it doesn't affect the metadata tables, but I'm not sure.)
Also, I saw a recent failure of RegexGroupBalancerIT, and I believe it has the same problem you were trying to fix here. Might be best to fix both. 😺
There was a problem hiding this comment.
So if you look closely at the ChoaticBalancer, it is looping over all tables. And then looking at the git history of the test, it makes sense that it was created to test across all tables. I think setting it to balance for certain table is what was also causing problems.
If we want to test per table load balancing then the ChaoticBalancer needs to be rewritten.
There was a problem hiding this comment.
After looking at the code more, I think there might be more fundamental problems with the ChaoticLoadBalancer, as it does not appear to be "per-table" aware, and will compete with other balancers trying to balance their tables. It probably is best to set this at site, like you did.... at least for now.
| @Deprecated | ||
| @Override | ||
| public void init(ServerConfiguration conf) { | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
This method has no impact at runtime inside the Accumulo master service. It would only affect somebody calling it directly... and that would probably only happen during testing. So, I think this exception would have been here to prevent testing bugs (accidentally calling the wrong init during testing). Might be best to leave it... though if our tests are correct, it should not matter.
There was a problem hiding this comment.
Yeah I can add it back.
There was a problem hiding this comment.
Thinking about it some more... the UnsupportedOperationException should really exist in the parent. Actually, I think we only kept this method around at all, because of API stability reasons... but this isn't public API, and I don't think it's very stable as is anyway. I would probably leave it as-is in previous release lines, and reconsider this whole API in 2.0 (or some other future point).
|
It seems the ChaoticLoadBalancer is ancient, predating modern man. It is a bug that it is not table aware even if it seems that wasn't the original intention. These fixes will at least enable using it as a viable IT. |
First fixed the NPE by removing the overloaded init methods. Then fixed the test (I think) by changing the configuration back to what it was originally intended, a balancer for all tables not just one.