You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Jake Maes <ja...@gmail.com> on 2016/08/17 15:35:58 UTC
Re: Review Request 50151: SAMZA-975 Initial Samza REST Implementation
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50151/
-----------------------------------------------------------
(Updated Aug. 17, 2016, 3:35 p.m.)
Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).
Bugs: SAMZA-975
https://issues.apache.org/jira/browse/SAMZA-975
Repository: samza
Description
-------
SAMZA-975 Initial Samza REST Implementation
Implementation closely reflects the design from SAMZA-865.
In addition it also includes a basic Monitor feature, which enables users to schedule arbitrary "monitor" logic to run periodically as part of the Samza REST Service.
Diffs (updated)
-----
LICENSE e1439fe71052a97f1e5a2e189a645ce3e7422a47
build.gradle 1d4eb74b1294318db8454631ddd0901596121ab2
gradle/dependency-versions.gradle 47c71bfde027835682889407261d4798b629d214
samza-rest/src/main/bash/run-samza-rest-service.sh PRE-CREATION
samza-rest/src/main/config/samza-rest.properties PRE-CREATION
samza-rest/src/main/java/org/apache/samza/monitor/Monitor.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/monitor/MonitorLoader.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/monitor/ScheduledExecutorSchedulingProvider.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/monitor/SchedulingProvider.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/SamzaRestApplication.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/model/Job.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/model/JobStatus.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/installation/InstallationFinder.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/installation/InstallationRecord.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/installation/SimpleInstallationFinder.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobInstance.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobStatusProvider.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/ScriptJobProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/YarnCliJobStatusProvider.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/script/ScriptOutputHandler.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/script/ScriptPathProvider.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/script/ScriptRunner.java PRE-CREATION
samza-rest/src/main/resources/log4j.xml PRE-CREATION
samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/monitor/mock/DummyMonitor.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/monitor/mock/ExceptionThrowingMonitor.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/monitor/mock/InstantSchedulingProvider.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockJobProxy.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockJobProxyFactory.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockJobStatusProvider.java PRE-CREATION
samza-shell/src/main/bash/kill-yarn-job-by-name.sh PRE-CREATION
settings.gradle 4c1aa107a11d413777e69bc4e48847b811aff7d2
Diff: https://reviews.apache.org/r/50151/diff/
Testing
-------
Unit tests pass.
Deployed on a local cluster and verified the hello-samza jobs are listed at localhost:9139/v1/jobs
The service has been deployed for months in LinkedIn with additional Resources and Monitors.
Also ran bin/check-all.sh
Thanks,
Jake Maes
Re: Review Request 50151: SAMZA-975 Initial Samza REST Implementation
Posted by Jake Maes <ja...@gmail.com>.
> On Aug. 23, 2016, 1:21 a.m., Navina Ramesh wrote:
> > samza-rest/src/main/java/org/apache/samza/monitor/MonitorLoader.java, line 23
> > <https://reviews.apache.org/r/50151/diff/2/?file=1476792#file1476792line23>
> >
> > I wonder if it will be useful to have this as a more generic Utility class:
> > ```
> > class ClassLoaderHelper<T> {
> > ...
> > public static T fromClassName(String klassName) {
> > ...
> > return (T) instance;
> > }
> >
> > }
> > ```
I think so. In fact we have this pattern in more than a few locations in the Samza codebase.
How about we log a separate ticket to refactor all of those?
> On Aug. 23, 2016, 1:21 a.m., Navina Ramesh wrote:
> > build.gradle, line 513
> > <https://reviews.apache.org/r/50151/diff/1/?file=1446054#file1446054line513>
> >
> > Curious: are we creating 2 tar tasks because we want all the bash scripts from samza-shell as a part of the rest tar ball?
It's been a while so I'm not sure I recall perfectly, but I think the goal was to reuse some of the shell scripts rather than duplicating them and then rolling them together in one artifact.
So, yes. :-)
> On Aug. 23, 2016, 1:21 a.m., Navina Ramesh wrote:
> > samza-rest/src/main/config/samza-rest.properties, line 23
> > <https://reviews.apache.org/r/50151/diff/1/?file=1446057#file1446057line23>
> >
> > Wonder if there is a way to avoid this exact path and simply use an environment (system) variable. Just a thought. users need to configure it anyway.
> > Or is it possible make "$PWD/.." to be deafult? Would it work ?
I figured /export/content/samza/deploy would be a reasonable convention that might be a little more reusable than something like PWD or user.home. But I don't feel passionately about it. I can change it if you think it's better to pick something more relative.
Either way, they'll likely change it.
- Jake
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50151/#review145963
-----------------------------------------------------------
On Aug. 17, 2016, 3:35 p.m., Jake Maes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50151/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2016, 3:35 p.m.)
>
>
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).
>
>
> Bugs: SAMZA-975
> https://issues.apache.org/jira/browse/SAMZA-975
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-975 Initial Samza REST Implementation
>
> Implementation closely reflects the design from SAMZA-865.
> In addition it also includes a basic Monitor feature, which enables users to schedule arbitrary "monitor" logic to run periodically as part of the Samza REST Service.
>
>
> Diffs
> -----
>
> LICENSE e1439fe71052a97f1e5a2e189a645ce3e7422a47
> build.gradle 1d4eb74b1294318db8454631ddd0901596121ab2
> gradle/dependency-versions.gradle 47c71bfde027835682889407261d4798b629d214
> samza-rest/src/main/bash/run-samza-rest-service.sh PRE-CREATION
> samza-rest/src/main/config/samza-rest.properties PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/monitor/Monitor.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/monitor/MonitorLoader.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/monitor/ScheduledExecutorSchedulingProvider.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/monitor/SchedulingProvider.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/SamzaRestApplication.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Job.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/JobStatus.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/installation/InstallationFinder.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/installation/InstallationRecord.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/installation/SimpleInstallationFinder.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobInstance.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobStatusProvider.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/ScriptJobProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/YarnCliJobStatusProvider.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/script/ScriptOutputHandler.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/script/ScriptPathProvider.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/script/ScriptRunner.java PRE-CREATION
> samza-rest/src/main/resources/log4j.xml PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/monitor/mock/DummyMonitor.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/monitor/mock/ExceptionThrowingMonitor.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/monitor/mock/InstantSchedulingProvider.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockJobProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockJobProxyFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockJobStatusProvider.java PRE-CREATION
> samza-shell/src/main/bash/kill-yarn-job-by-name.sh PRE-CREATION
> settings.gradle 4c1aa107a11d413777e69bc4e48847b811aff7d2
>
> Diff: https://reviews.apache.org/r/50151/diff/
>
>
> Testing
> -------
>
> Unit tests pass.
> Deployed on a local cluster and verified the hello-samza jobs are listed at localhost:9139/v1/jobs
> The service has been deployed for months in LinkedIn with additional Resources and Monitors.
> Also ran bin/check-all.sh
>
>
> Thanks,
>
> Jake Maes
>
>
Re: Review Request 50151: SAMZA-975 Initial Samza REST Implementation
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50151/#review145963
-----------------------------------------------------------
Ship it!
Overall looks good. Just jotted down some thoughts :) +1
build.gradle (line 513)
<https://reviews.apache.org/r/50151/#comment212318>
Curious: are we creating 2 tar tasks because we want all the bash scripts from samza-shell as a part of the rest tar ball?
samza-rest/src/main/config/samza-rest.properties (line 23)
<https://reviews.apache.org/r/50151/#comment212319>
Wonder if there is a way to avoid this exact path and simply use an environment (system) variable. Just a thought. users need to configure it anyway.
Or is it possible make "$PWD/.." to be deafult? Would it work ?
samza-rest/src/main/java/org/apache/samza/monitor/MonitorLoader.java (line 23)
<https://reviews.apache.org/r/50151/#comment212921>
I wonder if it will be useful to have this as a more generic Utility class:
```
class ClassLoaderHelper<T> {
...
public static T fromClassName(String klassName) {
...
return (T) instance;
}
}
```
- Navina Ramesh
On Aug. 17, 2016, 3:35 p.m., Jake Maes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50151/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2016, 3:35 p.m.)
>
>
> Review request for samza, Boris Shkolnik, Chris Pettitt, Jake Maes, Navina Ramesh, Jagadish Venkatraman, Xinyu Liu, and Yi Pan (Data Infrastructure).
>
>
> Bugs: SAMZA-975
> https://issues.apache.org/jira/browse/SAMZA-975
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-975 Initial Samza REST Implementation
>
> Implementation closely reflects the design from SAMZA-865.
> In addition it also includes a basic Monitor feature, which enables users to schedule arbitrary "monitor" logic to run periodically as part of the Samza REST Service.
>
>
> Diffs
> -----
>
> LICENSE e1439fe71052a97f1e5a2e189a645ce3e7422a47
> build.gradle 1d4eb74b1294318db8454631ddd0901596121ab2
> gradle/dependency-versions.gradle 47c71bfde027835682889407261d4798b629d214
> samza-rest/src/main/bash/run-samza-rest-service.sh PRE-CREATION
> samza-rest/src/main/config/samza-rest.properties PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/monitor/Monitor.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/monitor/MonitorLoader.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/monitor/SamzaMonitorService.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/monitor/ScheduledExecutorSchedulingProvider.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/monitor/SchedulingProvider.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/SamzaRestApplication.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/SamzaRestConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/SamzaRestService.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Job.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/JobStatus.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/installation/InstallationFinder.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/installation/InstallationRecord.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/installation/SimpleInstallationFinder.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobInstance.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobStatusProvider.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/ScriptJobProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/YarnCliJobStatusProvider.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/script/ScriptOutputHandler.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/script/ScriptPathProvider.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/script/ScriptRunner.java PRE-CREATION
> samza-rest/src/main/resources/log4j.xml PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/monitor/TestMonitorService.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/monitor/mock/DummyMonitor.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/monitor/mock/ExceptionThrowingMonitor.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/monitor/mock/InstantSchedulingProvider.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockJobProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockJobProxyFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockJobStatusProvider.java PRE-CREATION
> samza-shell/src/main/bash/kill-yarn-job-by-name.sh PRE-CREATION
> settings.gradle 4c1aa107a11d413777e69bc4e48847b811aff7d2
>
> Diff: https://reviews.apache.org/r/50151/diff/
>
>
> Testing
> -------
>
> Unit tests pass.
> Deployed on a local cluster and verified the hello-samza jobs are listed at localhost:9139/v1/jobs
> The service has been deployed for months in LinkedIn with additional Resources and Monitors.
> Also ran bin/check-all.sh
>
>
> Thanks,
>
> Jake Maes
>
>