----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9166/#review15912 -----------------------------------------------------------
bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/shell/Shell.groovy <https://reviews.apache.org/r/9166/#comment34193> Hi Hari, this is when java/groovy starts to get hairy for me so I wrote a test in groovy. Here is how it looks: private void test(int x, Object... args){ System.out.println("with x") } private void test(Object... args){ System.out.println("without x") } test(1,2,3) test('a','b','c') test("abc") test(1) test(1, 'a') The output is: with x without x without x with x with x Conclusion: If the first argument of the call is int, the method with 1st int argument gets called (i.e. the signature you are adding) directly, bypassing the existing method that just takes Object... Do you see any concerns here? 1. Do you think those concerns may be resolved by making the newly signature private? 2. Even better, do you think there is a way to accomodate the additional timeout as a part of the existing arguments parameter without changing the method signature? - Mark Grover On Jan. 30, 2013, 8:35 p.m., Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9166/ > ----------------------------------------------------------- > > (Updated Jan. 30, 2013, 8:35 p.m.) > > > Review request for bigtop and Roman Shaposhnik. > > > Description > ------- > > Added new methods which can execute a script for a specified timeout and can > also execute in the background. > > > This addresses bug BIGTOP-835. > https://issues.apache.org/jira/browse/BIGTOP-835 > > > Diffs > ----- > > > bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/shell/Shell.groovy > ae3da68 > > bigtop-test-framework/src/test/groovy/org/apache/bigtop/itest/shell/ShellTest.groovy > 1571e10 > > Diff: https://reviews.apache.org/r/9166/diff/ > > > Testing > ------- > > Added unit tests for new functionality. Current unit tests pass > > > Thanks, > > Hari Shreedharan > >
