[GitHub] ctubbsii commented on a change in pull request #401: Fix ChaoticBalancerIT

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #401: Fix ChaoticBalancerIT
URL: https://github.com/apache/accumulo/pull/401#discussion_r174987369
 
 

 ##########
 File path: test/src/test/java/org/apache/accumulo/test/functional/ChaoticBalancerIT.java
 ##########
 @@ -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());
 
 Review 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. :smiley_cat: 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services