Skip to content

KAFKA-7828: Add ExternalCommandWorker to Trogdor#6219

Merged
cmccabe merged 2 commits intoapache:trunkfrom
cmccabe:KAFKA-7828-II
Feb 7, 2019
Merged

KAFKA-7828: Add ExternalCommandWorker to Trogdor#6219
cmccabe merged 2 commits intoapache:trunkfrom
cmccabe:KAFKA-7828-II

Conversation

@cmccabe
Copy link
Copy Markdown
Contributor

@cmccabe cmccabe commented Feb 1, 2019

Allow the Trogdor agent to execute external commands. The agent communicates with the external commands via stdin, stdout, and stderr. Based on a patch by Xi Yang xi@confluent.io

@cmccabe cmccabe changed the title KAFKA-7828: Add ExternalCommandWorker KAFKA-7828: Add ExternalCommandWorker to Trogdor Feb 1, 2019
@mumrah
Copy link
Copy Markdown
Member

mumrah commented Feb 1, 2019

Any thoughts on using a library like commons-exec or zt-exec?

@cmccabe
Copy link
Copy Markdown
Contributor Author

cmccabe commented Feb 1, 2019

A lot of the rationale for the commons-exec library seems to have gone away with the newer JDKs. For example, it mentions that "in J2SE 1.1-1.4 there is not support for... [setting] environment variables". This is not an issue any more since the JDK's ProcessBuilder now allows setting up the environment. This might be one reason why the last published release of the commons-exec library was from 2014 and mentions Java 1.5.

zt-exec seems a little more active (it had a release less than a year ago) but still doesn't seem to really solve for the things that I want. Again, a lot of the stuff it mentions is already in the standard java library. For example-- "Redirecting stderr to stdout"-- Java's ProcessBuilder can now do this easily. And so on.

In general, most of the complexity in this patch is about dealing with event streams coming into and out of the process. That would be the same even if a different library were used. The JDK's library is the best maintained of any of these libraries, so I think it's the clear choice.

The features that I really want that are missing from the the JDK's library are:

  1. The ability to put the child process' stdout into line-buffered mode by default (Java defaults it to fully buffered)
  2. The ability to send signals to any children of the subprocess we're running.

#1 is not really a big deal since the subprocess can just flush its stdout as needed, or set line-buffered mode on its own. Also, I don't think any of the other libraries have this.

#2 is something we could probably hack up by running a shell command rather than using Process#destroy. But it's probably not really needed unless people want to run bash scripts through this interface (bash loves spawning tons of subprocesses.) For running python or C, it's not really needed. And again, I doubt any other library provides something like this.

Copy link
Copy Markdown
Member

@mumrah mumrah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bunch of small stuff

Comment thread tests/bin/external_trogdor_command_example.py Outdated
Comment thread tests/spec/external_command.json Outdated
Comment thread tools/src/main/java/org/apache/kafka/trogdor/workload/ExternalCommandSpec.java Outdated
Comment thread tools/src/main/java/org/apache/kafka/trogdor/workload/ExternalCommandWorker.java Outdated
Comment thread tools/src/main/java/org/apache/kafka/trogdor/workload/ExternalCommandWorker.java Outdated
@mumrah
Copy link
Copy Markdown
Member

mumrah commented Feb 4, 2019

Thanks for the explanation @cmccabe. Left a review with several small comments/questions.

Copy link
Copy Markdown
Member

@mumrah mumrah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cmccabe cmccabe merged commit 4be68c5 into apache:trunk Feb 7, 2019
@cmccabe cmccabe deleted the KAFKA-7828-II branch May 20, 2019 18:54
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
Allow the Trogdor agent to execute external commands. The agent communicates with the external commands via stdin, stdout, and stderr.

Based on a patch by Xi Yang <xi@confluent.io>

Reviewers: David Arthur <mumrah@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants