Flume, mail # dev - Re: Review Request: Added new config 'shell' for Exec source

Review Request: Added new config 'shell' for Exec source

Roshan Naik 2012-10-26, 18:15

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7748/
-----------------------------------------------------------

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.
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-doc/sphinx/FlumeUserGuide.rst 29ead84

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

Re: Review Request: Added new config 'shell' for Exec source

Roshan Naik 2012-11-03, 00:02

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7748/
-----------------------------------------------------------

(Updated Nov. 3, 2012, 12:02 a.m.)
Review request for Flume.
Changes
-------

linking review to jira
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-doc/sphinx/FlumeUserGuide.rst 29ead84

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

Re: Review Request: Added new config 'shell' for Exec source

Roshan Naik 2012-11-06, 21:52

-----------------------------------------------------------
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.
Changes
-------

Consolidating feedback from all comments into this patch.
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 (updated)
-----

  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

Re: Review Request: Added new config 'shell' for Exec source

Alexander Alten-Lorenz 2013-01-24, 10:43

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7748/#review15639
-----------------------------------------------------------

Ship it!
Ship It!

- Alexander Alten-Lorenz
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
>
>

Re: Review Request: Added new config 'shell' for Exec source

Brock Noland 2013-01-28, 19:27

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7748/#review15760
-----------------------------------------------------------
Sorry for being so tardy. Below is a few review items, oeverall I think the patch looks quite good!
flume-ng-core/src/main/java/org/apache/flume/source/ExecSource.java
<https://reviews.apache.org/r/7748/#comment33919>

   nit: I understand the other member variables are not final, but since we are adding a new one, can we make this one final?

flume-ng-core/src/test/java/org/apache/flume/source/TestExecSource.java
<https://reviews.apache.org/r/7748/#comment33918>

   +1 for moving this to shared methods. However, I cannot think of any other tests except the HBase tests which use static stuff. I think it makes sense if the static stuff is heavy weight but these objects are light weight. Let's stick with Before and After here.

flume-ng-core/src/test/resources/test_command.txt
<https://reviews.apache.org/r/7748/#comment33916>

   Let's add tests to test shell arithmetic and also the more modern $( ) proces substitution.

flume-ng-doc/sphinx/FlumeUserGuide.rst
<https://reviews.apache.org/r/7748/#comment33917>

   I like the term Required over Needed here.

   Also, typically the term backtick is used over back tick.
- Brock Noland
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
>
>

Re: Review Request: Added new config 'shell' for Exec source

Brock Noland 2013-03-06, 12:55

> 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.

Hmm I might have highlighted the wrong one, I mean the member variable of ExecRunnable.
> On Jan. 28, 2013, 7:27 p.m., Brock Noland wrote:
> > flume-ng-core/src/test/java/org/apache/flume/source/TestExecSource.java, line 58
> > <https://reviews.apache.org/r/7748/diff/2/?file=186017#file186017line58>
> >
> > +1 for moving this to shared methods. However, I cannot think of any other tests except the HBase tests which use static stuff. I think it makes sense if the static stuff is heavy weight but these objects are light weight. Let's stick with Before and After here.
>
> Roshan Naik wrote:
>
> > 'it makes sense if the static stuff is heavy weight but these objects are light weight'
> I am not clear what heavy/light weight has to do with this being static.
>
> > Let's stick with Before and After here.
> I assume you are suggesting moving things from @BeforeClass to @Before. I dont see why we would want to de-optimize things by doing things multiple times when there is no need for it.

If it's static, it will only be initialized once so if it's heavy-weight that would speed up the tests considerably. However, these objects not heavy-weight and can be created in milliseconds so I prefer to not share them amongst the tests.
@Before and instance variables have the nice property that side affects from tests will not cause the tests to be in-determinate when running many tests in the same JVM. Unless there is a proven performance issue I'd prefer to keep everything as instance variables so we don't see things like "if test B runs after A or by itself it's fine, but if it runs after C it fails."
> 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.

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.
- 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:
>
> -----------------------------------------------------------
> 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.

Re: Review Request: Added new config 'shell' for Exec source

Roshan Naik 2013-03-06, 06:50

> 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?

This cannot be final.. its value is set by the configure method.
> On Jan. 28, 2013, 7:27 p.m., Brock Noland wrote:
> > flume-ng-core/src/test/java/org/apache/flume/source/TestExecSource.java, line 58
> > <https://reviews.apache.org/r/7748/diff/2/?file=186017#file186017line58>
> >
> > +1 for moving this to shared methods. However, I cannot think of any other tests except the HBase tests which use static stuff. I think it makes sense if the static stuff is heavy weight but these objects are light weight. Let's stick with Before and After here.
> 'it makes sense if the static stuff is heavy weight but these objects are light weight'
I am not clear what heavy/light weight has to do with this being static.

> Let's stick with Before and After here.
I assume you are suggesting moving things from @BeforeClass to @Before. I dont see why we would want to de-optimize things by doing things multiple times when there is no need for it.
> 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.

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.
- 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
>
>

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

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:

Re: Review Request: Added new config 'shell' for Exec source

Roshan Naik 2013-03-07, 19:25

Re: Review Request: Added new config 'shell' for Exec source

Brock Noland 2013-03-07, 20:45

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7748/#review17573
-----------------------------------------------------------
Looks good! Thanks Roshan! I was running tests to commit this and I get a couple failures. Below. Do they pass for you?

Tests in error:
testShellCommandEmbeddingAndEscaping(org.apache.flume.source.TestExecSource): flume-ng-core/src/test/resources/test_command.txt (No such file or directory)
testShellCommandEmbeddingAndEscaping(org.apache.flume.source.TestExecSource)

- Brock Noland
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
>
>

Re: Review Request: Added new config 'shell' for Exec source

Brock Noland 2013-03-11, 19:17

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7748/#review17697
-----------------------------------------------------------

Ship it!
Ship It!

- Brock Noland
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
>
>

Re: Review Request: Added new config 'shell' for Exec source

Brock Noland 2012-11-03, 03:51

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7748/#review13073
-----------------------------------------------------------

flume-ng-core/src/test/java/org/apache/flume/source/TestExecSource.java
<https://reviews.apache.org/r/7748/#comment28130>

Let's not hard code /tmp. Files.createTempDir() works well/
- Brock Noland
On Nov. 3, 2012, 12:02 a.m., Roshan Naik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7748/
> -----------------------------------------------------------
>
> (Updated Nov. 3, 2012, 12:02 a.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-doc/sphinx/FlumeUserGuide.rst 29ead84
>
> 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
>
>

Re: Review Request: Added new config 'shell' for Exec source

Roshan Naik 2012-11-06, 21:38

> On Nov. 3, 2012, 3:51 a.m., Brock Noland wrote:
> > flume-ng-core/src/test/java/org/apache/flume/source/TestExecSource.java, line 188
> > <https://reviews.apache.org/r/7748/diff/1/?file=180442#file180442line188>
> >
> > Let's not hard code /tmp. Files.createTempDir() works well/

fixed in v4 patch
- Roshan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7748/#review13073
-----------------------------------------------------------
On Nov. 3, 2012, 12:02 a.m., Roshan Naik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7748/
> -----------------------------------------------------------
>
> (Updated Nov. 3, 2012, 12:02 a.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-doc/sphinx/FlumeUserGuide.rst 29ead84
>
> 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
>
>

Re: Review Request: Added new config 'shell' for Exec source

Jarek Cecho 2012-10-26, 18:58

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7748/#review12839
-----------------------------------------------------------
Would you mind improving user guide a bit more? I would appreciate some paragraphs about the differences when shell is/isn't included and what are use cases, etc...

- Jarek Cecho
On Oct. 26, 2012, 6:15 p.m., Roshan Naik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7748/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2012, 6:15 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.
>
>
> 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-doc/sphinx/FlumeUserGuide.rst 29ead84
>
> 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
>
>

Re: Review Request: Added new config 'shell' for Exec source

Roshan Naik 2012-11-03, 00:03

> On Oct. 26, 2012, 6:58 p.m., Jarek Cecho wrote:
> > Would you mind improving user guide a bit more? I would appreciate some paragraphs about the differences when shell is/isn't included and what are use cases, etc...

The revised patch (v2) contains the additional user documentation.
- Roshan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7748/#review12839
-----------------------------------------------------------
On Nov. 3, 2012, 12:02 a.m., Roshan Naik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7748/
> -----------------------------------------------------------
>
> (Updated Nov. 3, 2012, 12:02 a.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-doc/sphinx/FlumeUserGuide.rst 29ead84
>
> 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
>
>

Re: Review Request: Added new config 'shell' for Exec source

Mike Percy 2012-11-02, 10:02

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7748/#review13034
-----------------------------------------------------------
Hey Roshan, we should add a pointer back to https://issues.apache.org/jira/browse/FLUME-1661

Also we need a more complex test case to ensure escaping of double-quotes and nested quotes works. Would be good to read those values from a properties file (we can check it into src/test/resources) so that the string decoding is the same as it would be with a Flume config file in normal use.

- Mike Percy
On Oct. 26, 2012, 6:15 p.m., Roshan Naik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7748/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2012, 6:15 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.
>
>
> 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-doc/sphinx/FlumeUserGuide.rst 29ead84
>
> 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
>
>

Re: Review Request: Added new config 'shell' for Exec source

Roshan Naik 2012-11-03, 00:01

> On Nov. 2, 2012, 10:02 a.m., Mike Percy wrote:
> > Hey Roshan, we should add a pointer back to https://issues.apache.org/jira/browse/FLUME-1661
> >
> > Also we need a more complex test case to ensure escaping of double-quotes and nested quotes works. Would be good to read those values from a properties file (we can check it into src/test/resources) so that the string decoding is the same as it would be with a Flume config file in normal use.

Mike, I am not clear about what values we should read from a properties file for string decoding. Would be great if you could provide me with some context here.

In this patch the verbatim command is passed to the shell for execution. There is no decoding, escaping or any other processing performed on the command string in java. I can add another test case for nested quotes (single/double)
- Roshan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7748/#review13034
-----------------------------------------------------------
On Oct. 26, 2012, 6:15 p.m., Roshan Naik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7748/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2012, 6:15 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.
>
>
> 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-doc/sphinx/FlumeUserGuide.rst 29ead84
>
> 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
>
>

Re: Review Request: Added new config 'shell' for Exec source

Roshan Naik 2012-11-03, 02:31

> On Nov. 2, 2012, 10:02 a.m., Mike Percy wrote:
> > Hey Roshan, we should add a pointer back to https://issues.apache.org/jira/browse/FLUME-1661
> >
> > Also we need a more complex test case to ensure escaping of double-quotes and nested quotes works. Would be good to read those values from a properties file (we can check it into src/test/resources) so that the string decoding is the same as it would be with a Flume config file in normal use.
>
> Roshan Naik wrote:
> Mike, I am not clear about what values we should read from a properties file for string decoding. Would be great if you could provide me with some context here.
>
> In this patch the verbatim command is passed to the shell for execution. There is no decoding, escaping or any other processing performed on the command string in java. I can add another test case for nested quotes (single/double)

never mind.. i see what you are saying. :-)
Patch updated (v3)
- Roshan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7748/#review13034
-----------------------------------------------------------
On Nov. 3, 2012, 12:02 a.m., Roshan Naik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7748/
> -----------------------------------------------------------
>
> (Updated Nov. 3, 2012, 12:02 a.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-doc/sphinx/FlumeUserGuide.rst 29ead84
>
> 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
>
>