Samza-1818: Cleaning up TestRunner Apis to incorporate LegacyTaskApplication and SamzaApplication #652
Conversation
…StreamApplication
Sanil15
changed the title
| /** | ||
| * Constructs a new {@link TestRunner} from following components | ||
| * @param app samza job implementing {@link StreamApplication} | ||
| * @param app samza job implementing {@link SamzaApplication} |
| Preconditions.checkState( | ||
| StreamTask.class.isAssignableFrom(taskClass) || AsyncStreamTask.class.isAssignableFrom(taskClass)); | ||
| return new TestRunner(taskClass); | ||
| public static TestRunner of(LegacyTaskApplication taskApp) { |
There was a problem hiding this comment.
Why do you need a separate API for LegacyTaskApplication? It should extend SamzaApplication too.
There was a problem hiding this comment.
Sure I can just expose one TestRunner#of(SamzaApplication ...), but this makes it more obvious for Legacy users to use this.
| } | ||
|
|
||
| /** | ||
| * Creates an instance of {@link TestRunner} for High Level/Fluent Samza Api |
There was a problem hiding this comment.
s/for High Level .../for a {@link SamzaApplication}/
|
|
||
| TestRunner | ||
| .of(MyStreamTestTask.class) | ||
| .of(new LegacyTaskApplication(MyStreamTestTask.class.getName())) |
There was a problem hiding this comment.
FWIW, please take a look at PR #642 , most of the changes here are already incorporated in that PR. Question here: any specific reason that you want to explicitly construct a LegacyTaskApplication here, instead of retaining the same API, but internally convert the MyStreamTestTask.class to a LegacyTaskApplication instance in TestRunner?
There was a problem hiding this comment.
We have three choices for Public Api
Option 1: TestRunner.of(SamzaApplication), TestRunner.of(AsyncStreamTask), TestRunner.of(StreamTask)
Option 2: TestRunner.of(SamzaApplication), TestRunner.of(LegacyTaskApplication),
Option 3: TestRunner.of(SamzaApplication), TestRunner.of(Class taskClass)
I prefer the option 2 makes it more straighforward for end user, option 3 is very generic and vague (which I want to remove)
I am ok with either Option 1 or Option 2. I will leave it up to the reviewers to suggest
Note: when i was read Samza-1818, i thought I am responsible for the refactor, if #642 gets merged first I can rebase the changes
There was a problem hiding this comment.
What's the specific concern on option 3? Type safety? We can add that in the TestRunner.of() method as well. If your concern is that what end user will see as a supported implementation of application, we already support the legacy case that end user only implements the task class anyways and I don't think that will add additional confusion. In fact, exposing the constructor of LegacyTaskApplication is worse, since we don't expect users construct the LegacyTaskApplication at all in the supported APIs.
There was a problem hiding this comment.
Sure, will revert the changes
| * @param taskClass represent a class containing Samza job logic extending either {@link StreamTask} or {@link AsyncStreamTask} | ||
| * @param taskClass containing Samza job logic extending either {@link StreamTask} or {@link AsyncStreamTask} | ||
| */ | ||
| private TestRunner(Class taskClass) { |
There was a problem hiding this comment.
Is this method still required?
3 participants
No description provided.