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

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


+
Roshan Naik 2012-10-26, 18:15
+
Mike Percy 2012-11-02, 10:02
+
Roshan Naik 2012-11-03, 00:01
+
Roshan Naik 2012-11-03, 02:31
+
Roshan Naik 2012-11-03, 00:02
+
Brock Noland 2012-11-03, 03:51
+
Roshan Naik 2012-11-06, 21:38
+
Roshan Naik 2012-11-06, 21:52
+
Alexander Alten-Lorenz 2013-01-24, 10:43
+
Brock Noland 2013-01-28, 19:27
+
Roshan Naik 2013-03-06, 06:50
+
Brock Noland 2013-03-06, 12:55
+
Roshan Naik 2013-03-06, 21:08
Copy link to this message
-
Re: Review Request: Added new config 'shell' for Exec source
Brock Noland 2013-03-06, 23:03


> 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.
>
> Roshan Naik wrote:
>     That too cannot be final.  Also wasn't introduced in this patch.

This is a trivial point, but we seem to talking past each other and as such, I'd like to make sure we are on the same page. ExecRunnable->shell (line 248 in exec source) It's set in the constructor, why cannot it be final? It was also 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.
>
> Roshan Naik wrote:
>     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.  
>

Thank you for adding those tests and thank you for the tutorial! ;)

My concern was that the JVM uses execvp to exec processes. From the standard: "In the cases where the other members of the exec family of functions would fail and set errno to [ENOEXEC], the execlp() and execvp() functions shall execute a command interpreter" http://pubs.opengroup.org/onlinepubs/009604499/functions/exec.html reading that statement led me to believe this would result in bash -c "bash -c ''" when you did not specify the full path to bash in "shell" parameter as it could not exec it directly. However, I just did an strace and it does not. execvp just appears to search the path for the the command interpretor you specified.
- Brock
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:
+
Roshan Naik 2013-03-07, 19:25
+
Brock Noland 2013-03-07, 20:45
+
Brock Noland 2013-03-11, 19:17
+
Jarek Cecho 2012-10-26, 18:58
+
Roshan Naik 2012-11-03, 00:03