> On Jan. 28, 2013, 7:27 p.m., Brock Noland wrote: > > flume-ng-core/src/main/java/org/apache/flume/source/ExecSource.java, line > > 248 > > <https://reviews.apache.org/r/7748/diff/2/?file=186015#file186015line248> > > > > nit: I understand the other member variables are not final, but since > > we are adding a new one, can we make this one final? > > Roshan Naik wrote: > This cannot be final.. its value is set by the configure method. > > Brock Noland wrote: > Hmm I might have highlighted the wrong one, I mean the member variable of > ExecRunnable.
That too cannot be final. Also wasn't introduced in this patch. > On Jan. 28, 2013, 7:27 p.m., Brock Noland wrote: > > flume-ng-core/src/test/resources/test_command.txt, line 3 > > <https://reviews.apache.org/r/7748/diff/2/?file=186018#file186018line3> > > > > Let's add tests to test shell arithmetic and also the more modern $( ) > > proces substitution. > > Roshan Naik wrote: > Brock these are arithmetic and $( ) features are very specific to certain > shells. Although i dont mind adding more tests where needed .. it seems > futile to do these shell specific feature testing. > > The specified shell will process the whole command/script. If either $( > ) or shell arithmetic does not work.. we have a case for filing a bug > against that particular shell or maybe upgrading bash to a newer version. > > On a side note I have validated that this mechanism works on Windows with > powershell too. > > Brock Noland wrote: > I previously said I was not comfortable with this change so I'd like see > many tests showing this works as possible. I'd also argue that it's not > futile. When starting scripts via the -c mechanism it's almost always > quoting, escaping, or command substitution that cause issues. Note that these > features have been present in bash since 2004. I have added those tests. But I think your concern highlights an important mis-understanding on how this patch works. Perhaps my original response to this was not clear. So let me try again to address the concern. Assuming a user has a korn shell open and he user executes the following command $ bash -c "echo hello $some_var '$another_var' " The following things happen.. - 1) The interactive korn shell first does all the processing like processing quotes, substituting variables, handling wildcards etc. - a) the name of the executable to invoke.. in this case... bash - b) the actual arguments for the executable - 2) Now the korn shell performs a fork() or vfork() - 3) Next an exec() on the executable (i.e bash). The actual arguments from 1a are provided to exec. Now .. in this patch there is no step 1. We accept the shell name and command separately and there is NO processing of the latter. We do 2& 3 directly through Java. Hope that explains. - Roshan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7748/#review15760 ----------------------------------------------------------- On Nov. 6, 2012, 9:52 p.m., Roshan Naik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7748/ > ----------------------------------------------------------- > > (Updated Nov. 6, 2012, 9:52 p.m.) > > > Review request for Flume. > > > Description > ------- > > Added new optional config directive 'shell' for Exec Source. One can specify > how to invoke a shell to run the command (e.g. /bin/sh -c) . This is only > needed for commands that use features like wildcards, backticks, pipes, etc > that are supported by the shell. > > > This addresses bug https://issues.apache.org/jira/browse/FLUME-1661. > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FLUME-1661 > > > Diffs > ----- > > flume-ng-core/src/main/java/org/apache/flume/source/ExecSource.java 46f672f > > flume-ng-core/src/main/java/org/apache/flume/source/ExecSourceConfigurationConstants.java > 0ba0508 > flume-ng-core/src/test/java/org/apache/flume/source/TestExecSource.java > 8bcf320 > flume-ng-core/src/test/resources/test_command.txt PRE-CREATION > flume-ng-doc/sphinx/FlumeUserGuide.rst 3af400e > pom.xml 363c0e4 > > Diff: https://reviews.apache.org/r/7748/diff/ > > > Testing > ------- > > Have added a few test cases for both simple and more complex commands. > > > Thanks, > > Roshan Naik > >
