Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Flume, mail # dev - Review Request: Added new config 'shell' for Exec source


Copy link to this message
-
Re: Review Request: Added new config 'shell' for Exec source
Roshan Naik 2013-03-06, 21:08


> 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