KAFKA-7793: Improve the Trogdor command line. by cmccabe · Pull Request #6133 · apache/kafka

cmccabe · 2019-01-12T00:36:05Z

Allow the Trogdor agent to be started in "exec mode", where it simply
runs a single task and exits after it is complete.
For AgentClient and CoordinatorClient, allow the user to pass the path
to a file containing JSON, instead of specifying the JSON object in the
command-line text itself. This means that we can get rid of the bash
scripts whose only function was to load task specs into a bash string
and run a Trogdor command.
Print dates and times in a human-readable way, rather than as numbers
of milliseconds.
When listing tasks or workers, output human-readable tables of
information.
Allow the user to filter on task ID name, task ID pattern, or task
state.
Support a --json flag to provide raw JSON output if desired.

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

stanislavkozlovski

Lots of great changes! Thanks for the PR, @cmccabe!

I like the separation into .json specs a lot - it is more friendly for an abundance of examples rather than all the .sh scripts we had to duplicate logic in.

stanislavkozlovski · 2019-01-14T10:41:07Z

Do we support templating now?

Oops-- this was an oversight. Fixed.

stanislavkozlovski · 2019-01-14T10:42:07Z

I'm wondering whether we should add a README.md under /tests/spec/ or rename to something similar to /tests/trogdor_specs - just to make it clearer for new users what these specs represent

Good idea. I added an Apache header and a comment to the files, referring to TROGDOR.md

stanislavkozlovski · 2019-01-14T10:54:57Z

nit: Might as well create an addWorkerIdArgument() to be consistent with the other shared args

stanislavkozlovski · 2019-01-14T11:06:20Z

nit: probably not worth it to log an error, as another delete will be attempted. TestUtils.tempFile() calls file.deleteOnExit();

stanislavkozlovski · 2019-01-14T11:07:02Z

Those two test cases are exactly the same, I assume you wanted to test one with spaces ( ) only?

Fixed, thanks

stanislavkozlovski · 2019-01-14T11:57:01Z

nit: extra space

stanislavkozlovski · 2019-01-14T11:57:17Z

nit: extra space

stanislavkozlovski · 2019-01-14T11:58:37Z

Could we add a shorthand --s or chance --show-status's shorthand to --s?

Added -s as shorthand for --state

stanislavkozlovski · 2019-01-14T12:06:02Z

Will this work with multiple task ids? The tasks() code does this:

uriBuilder.queryParam("taskId", (Object[]) request.taskIds().toArray(new String[0]));

I think that returns a single-element array only?

List#toArray can return arrays with more than one element.

https://docs.oracle.com/javase/8/docs/api/java/util/List.html#toArray-T:A-

The array argument at the end is a little weird. TL;DR is that if you want to squeeze out some extra efficiency, you can pass something other than a zero-length array to that argument, to avoid object allocation.

stanislavkozlovski · 2019-01-14T12:06:11Z

This is great functionality but would it be better if we had it in the coordinator server in order to leverage it from REST API clients in other languages? Otherwise we probably need to duplicate logic.
We could maybe open a JIRA to track this as a separate item

We've been holding off on including regular expressions into APIs since we would like APIs to be language neutral, and Java regexps are not. They also change over time as Java changes (java 8 changed how some of them are interpreted).

I think we could open a follow-up JIRA for this, though. We can use re2 regular expressions to make the API language-neutral.

* Allow the Trogdor agent to be started in "exec mode", where it simply runs a single task and exits after it is complete. * For AgentClient and CoordinatorClient, allow the user to pass the path to a file containing JSON, instead of specifying the JSON object in the command-line text itself. This means that we can get rid of the bash scripts whose only function was to load task specs into a bash string and run a Trogdor command. * Print dates and times in a human-readable way, rather than as numbers of milliseconds. * When listing tasks or workers, output human-readable tables of information. * Allow the user to filter on task ID name, task ID pattern, or task state. * Support a --json flag to provide raw JSON output if desired.

mumrah · 2019-01-17T14:37:07Z

    }
+
+    /**
+     * Determine if a string is a JSON object literal.


Do we have any use cases for lists of objects at the top level of a JSON string?

That's a fair question. The answer here is that in all the cases where we're using this function, we want a JSON object as the top-level thing. We generally always make things objects in case we need to add more stuff later on. (For example, perhaps right now we want to configure X with a list of ints, but later we might want a list of ints and a float, etc.)

mumrah · 2019-01-17T14:39:35Z

+/**
+ * Utilities for formatting strings.
+ */
+public class Formatter {


Maybe StringFormatter?

Sounds good. Renamed

mumrah

I don't see any examples of --exec usage in the README. Maybe we could add one?

Supplying the spec JSON in a file rather than having to do shell gymnastics is definitely an improvement. Human readable output is always nice (for us humans, at least).

cmccabe · 2019-01-23T17:44:46Z

retest this please

mumrah

LGTM

stanislavkozlovski

JDK8 build test failure is unrelated - kafka.api.AdminClientIntegrationTest.testForceClose

LGTM! Working with Trogdor locally just got a heck of a lot better - thanks a lot for these changes @cmccabe!

* ak/trunk: MINOR: Update usage of deprecated API (apache#6146) KAFKA-4217: Add KStream.flatTransform (apache#5273) MINOR: Update Gradle to 5.1.1 (apache#6160) KAFKA-3522: Generalize Segments (apache#6170) Added quotes around the class path (apache#4469) KAFKA-7837: Ensure offline partitions are picked up as soon as possible when shrinking ISR (apache#6202) MINOR: In the MetadataResponse schema, ignorable should be a boolean KAFKA-7838: Log leader and follower end offsets when shrinking ISR (apache#6168) KAFKA-5692: Change PreferredReplicaLeaderElectionCommand to use Admin… (apache#3848) MINOR: clarify why suppress can sometimes drop tombstones (apache#6195) MINOR: Upgrade ducktape to 0.7.5 (apache#6197) MINOR: Improve IntegrationTestUtils documentation (apache#5664) MINOR: upgrade to jdk8 8u202 KAFKA-7693; Fix SequenceNumber overflow in producer (apache#5989) KAFKA-7692; Fix ProducerStateManager SequenceNumber overflow (apache#5990) MINOR: update copyright year in the NOTICE file. (apache#6196) KAFKA-7793: Improve the Trogdor command line. (apache#6133)

* Allow the Trogdor agent to be started in "exec mode", where it simply runs a single task and exits after it is complete. * For AgentClient and CoordinatorClient, allow the user to pass the path to a file containing JSON, instead of specifying the JSON object in the command-line text itself. This means that we can get rid of the bash scripts whose only function was to load task specs into a bash string and run a Trogdor command. * Print dates and times in a human-readable way, rather than as numbers of milliseconds. * When listing tasks or workers, output human-readable tables of information. * Allow the user to filter on task ID name, task ID pattern, or task state. * Support a --json flag to provide raw JSON output if desired. Reviewed-by: David Arthur <mumrah@gmail.com>, Stanislav Kozlovski <stanislav_kozlovski@outlook.com>

cmccabe mentioned this pull request Jan 12, 2019

Trogdor Improvements: execute Trogdor tasks directly and run the Trogdor worker in a separate process #6087

Closed

3 tasks

stanislavkozlovski reviewed Jan 14, 2019

View reviewed changes

cmccabe added 2 commits January 15, 2019 14:31

Fixes to spec files

ddc9f81

cmccabe force-pushed the KAFKA-7793 branch from 4c0b86b to ddc9f81 Compare January 15, 2019 22:34

Address review comments

e53d49c

mumrah reviewed Jan 17, 2019

View reviewed changes

cmccabe added 2 commits January 18, 2019 14:03

Formatter -> StringFormatter

b34e7d6

Add exec mode to TROGDOR.md

79faa3a

mumrah approved these changes Jan 23, 2019

View reviewed changes

stanislavkozlovski approved these changes Jan 24, 2019

View reviewed changes

cmccabe merged commit a79d6dc into apache:trunk Jan 24, 2019

cmccabe deleted the KAFKA-7793 branch May 20, 2019 19:03

Conversation

cmccabe commented Jan 12, 2019

Committer Checklist (excluded from commit message)

Uh oh!

stanislavkozlovski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mumrah left a comment

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Jan 23, 2019

Uh oh!

mumrah left a comment

Choose a reason for hiding this comment

Uh oh!

stanislavkozlovski left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants