----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16908/#review31928 -----------------------------------------------------------
src/server/src/main/java/org/apache/accumulo/server/monitor/servlets/DefaultServlet.java <https://reviews.apache.org/r/16908/#comment60679> Leave this change out, and put it in a follow on. src/server/src/main/java/org/apache/accumulo/server/test/functional/RunTests.java <https://reviews.apache.org/r/16908/#comment60680> 1) you should do this in an overridden setup method, rather than on each run through map. 2) Don't turn it into a file. Just use Configuration.getInt(key, default) (or the String equivalent) src/server/src/main/java/org/apache/accumulo/server/test/functional/RunTests.java <https://reviews.apache.org/r/16908/#comment60681> If accumulo_timeout_factor is already a string, there's no reason call toString src/server/src/main/java/org/apache/accumulo/server/test/functional/RunTests.java <https://reviews.apache.org/r/16908/#comment60682> I would recommend not changing this to varargs src/server/src/main/java/org/apache/accumulo/server/test/functional/RunTests.java <https://reviews.apache.org/r/16908/#comment60684> You should define "accumulo.timeout_factor" as a constant in the class, to ensure consistency. Also, you should probably base it on the Class name to make sure it's unique. e.g. static final String TIMEOUT_FACTOR = RunTests.class.getName() + ".timeout_factor"; src/server/src/main/java/org/apache/accumulo/server/test/functional/RunTests.java <https://reviews.apache.org/r/16908/#comment60683> likewise, I would not change this to varargs src/server/src/main/java/org/apache/accumulo/server/test/functional/RunTests.java <https://reviews.apache.org/r/16908/#comment60685> remove spurious String concatenation test/system/auto/README <https://reviews.apache.org/r/16908/#comment60686> suggest rewriting to: where 'timeout_factor' is an optional integer indicating how much we should scale up timeouts. Will be used both to set mapred.task.timeout and to set hte "-f" flag on run.py. The 'timeout_factor' defaults to 1, which corresponds to a mapred.task.timeout of 480 seconds. - Sean Busbey On Jan. 15, 2014, 5:20 p.m., Hung Pham wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16908/ > ----------------------------------------------------------- > > (Updated Jan. 15, 2014, 5:20 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-2005 > https://issues.apache.org/jira/browse/ACCUMULO-2005 > > > Repository: accumulo > > > Description > ------- > > ACCUMULO-2005: Add a 3rd argument to functional test RunTests.java to allow a > timeout factor for mapred.task.timeout and run.py -f flag. Also changed 'ex' > to 'e' on line 274 of > src/server/src/main/java/org/apache/accumulo/server/monitor/servlets/DefaultServlet.java > > > Diffs > ----- > > > src/server/src/main/java/org/apache/accumulo/server/monitor/servlets/DefaultServlet.java > eae0e81 > > src/server/src/main/java/org/apache/accumulo/server/test/functional/RunTests.java > 1a19bb5 > test/system/auto/README 4773393 > > Diff: https://reviews.apache.org/r/16908/diff/ > > > Testing > ------- > > Testing was done on small cluster (4 nodes: master + 3 workers) to ensure MR > job complete successfully instead of failing out due to timeouts that lead to > killed attempts. Test results can be provided upon request. > > > Thanks, > > Hung Pham > >
