You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by GitBox <gi...@apache.org> on 2020/07/02 21:40:07 UTC
[GitHub] [samza] kw2542 opened a new pull request #1400: SAMZA-2558: Refactor app.runner.class
kw2542 opened a new pull request #1400:
URL: https://github.com/apache/samza/pull/1400
Issues: app.runner.class config is hard coded across multple places in Samza.
Changes:
1. Move ApplicationRunners.java from samza-api to samza-core module while the package name stays.
2. Add app.runner.class and getAppRunnerClass method to ApplicationConfig.
3. Update occurrence of app.runner.class to ApplicationConfig.APP_RUNNER_CLASS.
API Changes:
Usage of ApplicationRunners need to depend on samza-core module instead of samza-api.
Upgrade Instructions:
Depend on samza-core instead of samza-api module.
Usage Instructions:
N/A
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] cameronlee314 commented on pull request #1400: SAMZA-2558: Refactor app.runner.class
Posted by GitBox <gi...@apache.org>.
cameronlee314 commented on pull request #1400:
URL: https://github.com/apache/samza/pull/1400#issuecomment-659075353
> > Why is `ApplicationRunners` being moved out of samza-api? Is it so it can depend on the `ApplicationConfig` class?
> > It seems to me like `ApplicationRunners` is intended to be part of the API so that it can be used to help launch apps. If we expect a class to be able to be called by users, then it should not be in samza-core, since users should not need to depend on samza-core in general.
> > The placement of classes in samza-api vs. samza-core is useful because we can have different evolution rules for different parts of code. We need to generally evolve samza-api classes in a backwards compatible way since they are API, but we have more flexibility for samza-core classes since they aren't considered part of the API.
>
> Good question. IMO, `ApplicationRunners` only contains a single method `getApplicationRunner` which constructs the an `ApplicationRunner` with reflection. I would expect `ApplicationRunnerUtil#invoke` is the actual method help users to launch application instead of `ApplicationRunners`. What do you think?
Hm, I'm not sure about `ApplicationRunnerUtil#invoke` being the main API method here. I think it is more flexible to pass the `SamzaApplication` directly. It looks like @nickpan47 initially added this `ApplicationRunners` class. Maybe check with him about this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] cameronlee314 commented on pull request #1400: SAMZA-2558: Refactor app.runner.class
Posted by GitBox <gi...@apache.org>.
cameronlee314 commented on pull request #1400:
URL: https://github.com/apache/samza/pull/1400#issuecomment-659042892
Why is `ApplicationRunners` being moved out of samza-api? Is it so it can depend on the `ApplicationConfig` class?
It seems to me like `ApplicationRunners` is intended to be part of the API so that it can be used to help launch apps. If we expect a class to be able to be called by users, then it should not be in samza-core, since users should not need to depend on samza-core in general.
The placement of classes in samza-api vs. samza-core is useful because we can have different evolution rules for different parts of code. We need to generally evolve samza-api classes in a backwards compatible way since they are API, but we have more flexibility for samza-core classes since they aren't considered part of the API.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] kw2542 commented on pull request #1400: SAMZA-2558: Refactor app.runner.class
Posted by GitBox <gi...@apache.org>.
kw2542 commented on pull request #1400:
URL: https://github.com/apache/samza/pull/1400#issuecomment-659051643
> Why is `ApplicationRunners` being moved out of samza-api? Is it so it can depend on the `ApplicationConfig` class?
> It seems to me like `ApplicationRunners` is intended to be part of the API so that it can be used to help launch apps. If we expect a class to be able to be called by users, then it should not be in samza-core, since users should not need to depend on samza-core in general.
> The placement of classes in samza-api vs. samza-core is useful because we can have different evolution rules for different parts of code. We need to generally evolve samza-api classes in a backwards compatible way since they are API, but we have more flexibility for samza-core classes since they aren't considered part of the API.
Good question. IMO, `ApplicationRunners` only contains a single method `getApplicationRunner` which constructs the an `ApplicationRunner` with reflection. I would expect `ApplicationRunnerUtil#invoke` is the actual method help users to launch application instead of `ApplicationRunners`. What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] nickpan47 commented on pull request #1400: SAMZA-2558: Refactor app.runner.class
Posted by GitBox <gi...@apache.org>.
nickpan47 commented on pull request #1400:
URL: https://github.com/apache/samza/pull/1400#issuecomment-689893301
lgtm. ApplicationRunners is only invoked in runtime. W/ the runner being configured, user code should not use this. Agreeing with the sentiment to move it to samza-core. Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] asfgit closed pull request #1400: SAMZA-2558: Refactor app.runner.class
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1400:
URL: https://github.com/apache/samza/pull/1400
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org