Refactor traffic replayer into three files #558
Conversation
…ime that it is closed as a first-class feature of the coreUtilities tracing library. A number of changes were made to existing interfaces to reduce the scope of 'naked' Spans so that it's easier to audit if they're being used directly to close spans. The getCurrentSpan() needs to be public because it's in an interface and used internally, but it should really be private and require friend access only within its package. However, I'm not sure how to do better guardrails in Java. Signed-off-by: Greg Schohn <[email protected]>
# Conflicts: # TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/TrafficReplayer.java
… from the business logic. Some of the new class's constructors have been simplified/streamlined a bit at the expense of not creating a JsonTransfomer automatically. Signed-off-by: Greg Schohn <[email protected]>
…class. TopLevel deals with the total lifecycle (shutdown) of critical resources while Core does the orchestration between various top-level components. Signed-off-by: Greg Schohn <[email protected]>
gregschohn
requested review from
AndreKurait,
chelma,
kartg,
lewijacn,
mikaylathompson,
okhasawn and
sumobrian
as code owners
There was a problem hiding this comment.
This to me, as it is, is a good improvement for creating a more focused and less intimidating main class for the Replayer. My thoughts after looking through were:
TrafficReplayer: Entry class, minimal with only argument parsing
TrafficReplayerTopLevel: Feels like TrafficReplayerLifecycle where we handle the lifecycle events (mainly cleanup) for the Replayer process. Possibly the setupShutdownHookForReplayer method could be here as well.
TrafficReplayerCore: Feels less defined, but contains core functions for kicking off different stages in the lifecycle of an incoming request, from receiving the request to outputting a finished tuple
There was a problem hiding this comment.
This overall appears to me as an improvement. At the stage in the project, I see it as low risk and a two-way door decision for if we decide to split up the interfaces differently in the future and this moves us in the right direction regardless.
I don't see anything that stands out to me as incorrect.
Description
(this PR should be diffed against PR-557)
Split up the TrafficReplayer class into
One part for command line parsing and being a java application
One "top-level" part that handles the lifetime/shutdown of resources being used by the application/class
A "core" part that takes those resources and coordinates them together so that those components together perform a replay.
Category Refactoring
Why these changes are required? The responsibilities of the top-level coordination for the TrafficReplayer class were getting too complex and too intertangled
What is the old behavior before changes and new behavior after changes? No changes.
Issues Resolved
Part of/prereq for https://opensearch.atlassian.net/browse/MIGRATIONS-1618
Testing
All existing tests pass
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.