-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement zilla dump command similar to tcpdump #121
Conversation
What's still needed here:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job making so much progress on this task, @bmaidics. 👍
In general though, the dump
command design needs to be more closely aligned with other zilla commands using the ZillaCommandSpi
, like ZillaTuneCommand
and not so heavily influenced by the command-log
internals, which does not implement the same pattern.
Your intention behind the reuse is good, but it seems to be interfering a bit with getting the most natural solution for zilla dump
.
I suggest you complete the implementation for zilla dump
without any reuse, copying whatever classes you need from command-log
such as the layouts, and cleaning up the rest of the implementation to simplify as if it would stand alone and reduce the coupling.
Let's treat this as the next checkpoint.
Then we can re-introduce the dependency from command-log
to command-dump
to reuse the layout implementations for example, leaving the other internals unmodified.
Later, we might be picking up the layout implementations from the zilla engine, so this approach will evolve naturally to remove them altogether from the individual command projects over time.
...-dump/src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/DumpCommand.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/ZillaDumpCommand.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/ZillaDumpCommand.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/ZillaDumpCommand.java
Show resolved
Hide resolved
.../src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/ZillaDumpCommand.java
Show resolved
Hide resolved
...mand-dump/src/test/java/io/aklivity/zilla/runtime/command/dump/internal/TestDumpCommand.java
Outdated
Show resolved
Hide resolved
...mand-dump/src/test/java/io/aklivity/zilla/runtime/command/dump/internal/TestDumpCommand.java
Outdated
Show resolved
Hide resolved
...mand-dump/src/test/java/io/aklivity/zilla/runtime/command/dump/internal/TestDumpCommand.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/ZillaDumpCommand.java
Show resolved
Hide resolved
.../src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/ZillaDumpCommand.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/ZillaDumpCommand.java
Outdated
Show resolved
Hide resolved
...mp/src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/LoggableStream.java
Outdated
Show resolved
Hide resolved
...mmand-dump/src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/Logger.java
Outdated
Show resolved
Hide resolved
...and-dump/src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/Handlers.java
Outdated
Show resolved
Hide resolved
...dump/src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/DumpHandlers.java
Outdated
Show resolved
Hide resolved
...dump/src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/DumpHandlers.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I built this PR locally, and modified zilla-examples
tcp.echo
example stack.yml
to use develop-SNAPSHOT
instead of latest
image, then followed the README to launch the stack.
I then executed the following to get a shell inside the zilla
docker service:
docker exec -it $(docker ps -q -f name=example_zilla) sh
When I then run ./zilla dump -v
or ./zilla dump -v -o zilla.pcap
the executable exits without producing any output nor error which is unexpected.
.../src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/ZillaDumpCommand.java
Outdated
Show resolved
Hide resolved
...p/src/test/java/io/aklivity/zilla/runtime/command/dump/internal/airline/TestDumpCommand.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/aklivity/zilla/runtime/command/dump/internal/airline/ZillaDumpCommand.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor change then we should be able to merge!
Fix #114