You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Shanthoosh Venkataraman <sa...@gmail.com> on 2016/09/22 20:49:55 UTC
Review Request 52168: Tasks endpoint to list the complete details of
all tasks related to a job
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/
-----------------------------------------------------------
Review request for samza.
Repository: samza
Description
-------
This patch contains the following changes
* Http get api to list the complete details of all the tasks that belongs to a job.
* Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
Diffs
-----
docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala ba38b5cfa4e61b5513ce38dd2be32438b62cd7ce
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
samza-core/src/main/scala/org/apache/samza/util/Util.scala 95a5aa0a23db2a890c19166b6031b2a3b96689f2
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockJobConfigFactory.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52168/diff/
Testing
-------
Manual and unit testing has been done to verify the apis.
Thanks,
Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review152412
-----------------------------------------------------------
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (lines 128 - 130)
<https://reviews.apache.org/r/52168/#comment221414>
As we'd discussed, I've removed setting JobId, JobName which is linkedin specific in this patch. Any deployment environment can have it's own ConfigFactory implementation where it rewrites configuration specific to it.
- Shanthoosh Venkataraman
On Oct. 10, 2016, 11:10 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2016, 11:10 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
> On Oct. 13, 2016, 12:55 a.m., Jake Maes wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 55
> > <https://reviews.apache.org/r/52168/diff/5/?file=1533539#file1533539line55>
> >
> > What is the value of the container name?
>
> Shanthoosh Venkataraman wrote:
> name of the samza container in which the task is running. With respect to current implementation, container name is unique within a job.
>
> Jake Maes wrote:
> Sorry, the question was poorly worded. How is container name useful? Do we need it? If so, couldn't we derive it from the container id?
It will be used for debugging purposes in the monitor/client. It logically belongs to the task hierarchy. To answer questions like finding list of containers running on a particular host. These questions could be answered from filter on the container name & preferred host. Adding this also in a way completes the entire task model.
Currently container name is of the form samza-container-{containerId}. Hence, we could derive it. But the logic used to generate container id is specific to samza job model generator and is bound to change. Hence deriving it might not be a good idea.
- Shanthoosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review152418
-----------------------------------------------------------
On Oct. 13, 2016, 11:57 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2016, 11:57 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
- Shanthoosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review152418
-----------------------------------------------------------
On Oct. 13, 2016, 11:57 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2016, 11:57 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Jake Maes <jm...@apache.org>.
> On Oct. 13, 2016, 12:55 a.m., Jake Maes wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 55
> > <https://reviews.apache.org/r/52168/diff/5/?file=1533539#file1533539line55>
> >
> > What is the value of the container name?
>
> Shanthoosh Venkataraman wrote:
> name of the samza container in which the task is running. With respect to current implementation, container name is unique within a job.
Sorry, the question was poorly worded. How is container name useful? Do we need it? If so, couldn't we derive it from the container id?
- Jake
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review152418
-----------------------------------------------------------
On Oct. 13, 2016, 11:57 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2016, 11:57 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
> On Oct. 13, 2016, 12:55 a.m., Jake Maes wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 55
> > <https://reviews.apache.org/r/52168/diff/5/?file=1533539#file1533539line55>
> >
> > What is the value of the container name?
name of the samza container in which the task is running. With respect to current implementation, container name is unique within a job.
- Shanthoosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review152418
-----------------------------------------------------------
On Oct. 13, 2016, 11:57 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2016, 11:57 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Jake Maes <jm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review152418
-----------------------------------------------------------
docs/learn/documentation/versioned/rest/resources/tasks.md (line 24)
<https://reviews.apache.org/r/52168/#comment221427>
Might be worth mentioning that this Resource builds on the JobsResource and is not intended to be used independently.
docs/learn/documentation/versioned/rest/resources/tasks.md (line 52)
<https://reviews.apache.org/r/52168/#comment221441>
For clarity, I think this property should be "preferredHost"
docs/learn/documentation/versioned/rest/resources/tasks.md (line 55)
<https://reviews.apache.org/r/52168/#comment221422>
What is the value of the container name?
docs/learn/documentation/versioned/rest/resources/tasks.md (line 96)
<https://reviews.apache.org/r/52168/#comment221426>
Does this line belong under the Configuration section?
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 125)
<https://reviews.apache.org/r/52168/#comment221433>
nit: Between here and line 101, we're now creating 2 instances of StreamMetadataCache where we originally had only 1.
This probably won't cause problems since the TTL is 0, but it's not ideal, particularly if the TTL is ever increased.
Ignore this if it's difficult to change. Just noting it for the sake of the other reviewers.
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala
<https://reviews.apache.org/r/52168/#comment221425>
Lets not move this to Util. How about JobConfig or a funtional class called Rewriters
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java (line 40)
<https://reviews.apache.org/r/52168/#comment221442>
Since this is a new factory, maybe we should learn from the shortcoming of the JobProxyFactory and actually pass the MetricsRegistryMap in for this one.
That'll allow us to add metrics to the TaskProxy later.
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java (line 157)
<https://reviews.apache.org/r/52168/#comment221443>
This is nice!
Just a nit: Instead of "ResourceUtil" -> something like "ResponseTemplates" or "Responses"
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java (line 54)
<https://reviews.apache.org/r/52168/#comment221444>
nit: It's confusing to have job installation path in as a property of the TaskResourceConfig.
Shouldn't it be BaseResourceConfig.CONFIG_JOB_INSTALLATIONS_PATH?
There are multiple occurrences of this.
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java (line 55)
<https://reviews.apache.org/r/52168/#comment221445>
Why should this be necessary for the jobs resource tests?
- Jake Maes
On Oct. 12, 2016, 10:54 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2016, 10:54 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Jake Maes <jm...@apache.org>.
> On Oct. 17, 2016, 9:38 p.m., Jake Maes wrote:
> >
One last thing that occurs to me. Since DefaultResourceFactory now includes both JobsResource and TasksResource, can you make sure the tutorial still works as written? We need every step to be exact s.t. users aren't confused.
http://samza.apache.org/learn/tutorials/latest/samza-rest-getting-started.html
> On Oct. 17, 2016, 9:38 p.m., Jake Maes wrote:
> > samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java, line 56
> > <https://reviews.apache.org/r/52168/diff/6/?file=1535268#file1535268line56>
> >
> > Why has this property been added to this test? It should not be mandatory.
>
> Shanthoosh Venkataraman wrote:
> If configuration for the list of resource factories is undefined, then it is defaulted to DefaultResourceFactory which loads all the resources. Here we would just want to load the JobsResource, hence I'm explicitly loading it with custom resource factory.
Ok, I see that now.
- Jake
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review152928
-----------------------------------------------------------
On Oct. 24, 2016, 10 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2016, 10 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/ConfigFactoryBuilder.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
> On Oct. 17, 2016, 9:38 p.m., Jake Maes wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 105
> > <https://reviews.apache.org/r/52168/diff/6/?file=1535245#file1535245line105>
> >
> > This comment still looks out of place. Maybe it should just be incorporated in the table.
It's present already as a part of the configuration table. First configuration `task.proxy.factory.class` is added with description and required field. I just added it above to re-instate that importance of that config.
> On Oct. 17, 2016, 9:38 p.m., Jake Maes wrote:
> > samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java, line 56
> > <https://reviews.apache.org/r/52168/diff/6/?file=1535268#file1535268line56>
> >
> > Why has this property been added to this test? It should not be mandatory.
If configuration for the list of resource factories is undefined, then it is defaulted to DefaultResourceFactory which loads all the resources. Here we would just want to load the JobsResource, hence I'm explicitly loading it with custom resource factory.
- Shanthoosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review152928
-----------------------------------------------------------
On Oct. 13, 2016, 11:57 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2016, 11:57 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Jake Maes <jm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review152928
-----------------------------------------------------------
docs/learn/documentation/versioned/rest/resources/tasks.md (line 23)
<https://reviews.apache.org/r/52168/#comment222117>
*used*
docs/learn/documentation/versioned/rest/resources/tasks.md (line 105)
<https://reviews.apache.org/r/52168/#comment222154>
This comment still looks out of place. Maybe it should just be incorporated in the table.
samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java (line 62)
<https://reviews.apache.org/r/52168/#comment222116>
This doesn't belong here. It has nothing to do with responses.
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java (line 55)
<https://reviews.apache.org/r/52168/#comment222151>
Why has this property been added to this test? It should not be mandatory.
- Jake Maes
On Oct. 13, 2016, 11:57 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2016, 11:57 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Jake Maes <jm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review152966
-----------------------------------------------------------
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 99)
<https://reviews.apache.org/r/52168/#comment222160>
shouldn't this line also use the new Util method to get the container name?
- Jake Maes
On Oct. 13, 2016, 11:57 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2016, 11:57 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Xinyu Liu <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review169231
-----------------------------------------------------------
Ship it!
Ship It!
- Xinyu Liu
On Feb. 16, 2017, 6:26 a.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2017, 6:26 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/java/org/apache/samza/standalone/StandaloneJobCoordinator.java 46dbf30eb37f7d617fb868fb4e1561fef18522d5
> samza-core/src/main/java/org/apache/samza/zk/ZkJobCoordinator.java PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/coordinator/JobModelManager.scala 14d5dff00758c6e35cae018b4ebaa686d67bb57d
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala 9019d02bc83e0e76a1b6a6fb9958733dfe8b86a4
> samza-core/src/test/java/org/apache/samza/execution/TestExecutionPlanner.java PRE-CREATION
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/52168/diff/15/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Xinyu Liu <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review169086
-----------------------------------------------------------
Please rebase this patch with master.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 64 (original), 78 (patched)
<https://reviews.apache.org/r/52168/#comment241409>
Add java doc indicating we not only read jobmodel but update some other stuff.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 120 (original), 155 (patched)
<https://reviews.apache.org/r/52168/#comment241408>
make this private and don't expose it outside.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 188 (original), 224 (patched)
<https://reviews.apache.org/r/52168/#comment241407>
rename this as getJobModle. In javadoc, make clear there is no side effect.
- Xinyu Liu
On Feb. 16, 2017, 6:26 a.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2017, 6:26 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/52168/diff/14/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Prateek Maheshwari via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review168665
-----------------------------------------------------------
Ship it!
Looks good to me, thanks.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 185 (original), 219 (patched)
<https://reviews.apache.org/r/52168/#comment240927>
"Reads the "
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Lines 221 (patched)
<https://reviews.apache.org/r/52168/#comment240928>
Let's just delete this line.
- Prateek Maheshwari
On Feb. 15, 2017, 10:26 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2017, 10:26 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/52168/diff/14/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Prateek Maheshwari via Review Board <no...@reviews.apache.org>.
> On Feb. 23, 2017, 12:24 p.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md
> > Lines 119 (patched)
> > <https://reviews.apache.org/r/52168/diff/12/?file=1637007#file1637007line119>
> >
> > The next two configs seem like JobsResource configs. If they are, shouldn't duplicate documentation.
>
> Shanthoosh Venkataraman wrote:
> It's done to group all the configuration required for task resource at a single point. Duplication was intended here(To group configuration related to TaskResource at a single point). Moving all the configuration common for the samza-rest resources in a seperate place could be done later.
Let's create a ticket for that then.
> On Feb. 23, 2017, 12:24 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/model/Task.java
> > Lines 130 (patched)
> > <https://reviews.apache.org/r/52168/diff/12/?file=1637013#file1637013line130>
> >
> > storeNames?
Doesn't look like this is fixed? To clarify, should include the storeName hashCode too.
> On Feb. 23, 2017, 12:24 p.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java
> > Line 108 (original), 107 (patched)
> > <https://reviews.apache.org/r/52168/diff/12/?file=1637025#file1637025line108>
> >
> > Responses.serverErrorResponse
>
> Shanthoosh Venkataraman wrote:
> I think errorResponse in itself is explanatory & sufficient. Don't understand the value in renaming to serverErrorResponse.
Error is a generic term, BadRequest/NotFound is also an error. Server error is specifically HTTP 5xx codes, which is what this sends.
- Prateek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review166245
-----------------------------------------------------------
On Feb. 15, 2017, 10:26 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2017, 10:26 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/52168/diff/13/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Prateek Maheshwari <pm...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review166245
-----------------------------------------------------------
First pass, will look at JobCoordinator changes separately.
docs/learn/documentation/versioned/rest/resource-directory.md (line 27)
<https://reviews.apache.org/r/52168/#comment238537>
I think you need a corresponding entry here.
docs/learn/documentation/versioned/rest/resources/tasks.md (line 22)
<https://reviews.apache.org/r/52168/#comment238497>
docs/learn/documentation/versioned/rest/resources/tasks.md (line 22)
<https://reviews.apache.org/r/52168/#comment238498>
"endpoints"
s/related to/for
docs/learn/documentation/versioned/rest/resources/tasks.md (line 23)
<https://reviews.apache.org/r/52168/#comment238499>
s/built on top of/is a sub-resource of the/. Second half ("and is not") is redundant.
docs/learn/documentation/versioned/rest/resources/tasks.md (line 41)
<https://reviews.apache.org/r/52168/#comment238500>
"the complete" is redundant.
s/that belongs to/for
docs/learn/documentation/versioned/rest/resources/tasks.md (line 74)
<https://reviews.apache.org/r/52168/#comment238501>
s/had completed/completed
Second half is unnecessary. If you keep it, use past tense to be consistent with the first�half. Also s/belong to/for
docs/learn/documentation/versioned/rest/resources/tasks.md (line 80)
<https://reviews.apache.org/r/52168/#comment238502>
Space after colon.
Let's not introduce a new convention, let's either use "jobName" or "job name". Update the place where this message is generated too.
docs/learn/documentation/versioned/rest/resources/tasks.md (line 101)
<https://reviews.apache.org/r/52168/#comment238419>
"central point to interact".
Is this for interacting with the task or just getting information about it? If the latter, should clarify it here and elsewhere.
Document intention, not implementation, for interfaces. E.g., "For example, it allows getting information about all tasks for the job" vs "exposes a method to ...".
docs/learn/documentation/versioned/rest/resources/tasks.md (line 116)
<https://reviews.apache.org/r/52168/#comment238509>
Not sure what you mean by "uses the SimpleInstallationRecord". If InstallationFinder is configurable, this shouldn't be referring to a particular installation record type. Do all InstallationFinders return a SimpleInstallationRecored? If that's the case, should remove "Simple" from the name.
docs/learn/documentation/versioned/rest/resources/tasks.md (line 119)
<https://reviews.apache.org/r/52168/#comment238539>
The next two configs seem like JobsResource configs. If they are, shouldn't duplicate documentation.
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java (line 28)
<https://reviews.apache.org/r/52168/#comment238420>
Delete extra newline at end of documentation block, here and elsewhere.
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java (line 28)
<https://reviews.apache.org/r/52168/#comment238421>
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java (line 86)
<https://reviews.apache.org/r/52168/#comment238154>
Minor: Prefer:
Partition other = (Partition) o;
return this.partitionId == other.partitionId &&
this.system == other.system &&
this.stream == other.stream;
Same elsewhere.
samza-rest/src/main/java/org/apache/samza/rest/model/Task.java (line 130)
<https://reviews.apache.org/r/52168/#comment238512>
storeNames?
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 57)
<https://reviews.apache.org/r/52168/#comment238541>
Not sure what "cluster agnostic" means here. Should document details about this particular implementation, e.g. that it gets the Task information and Config from the job coordinator stream.
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 77)
<https://reviews.apache.org/r/52168/#comment238416>
This class is classloading its own factory from config to create an instance of itself? Why is this required?
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 120)
<https://reviews.apache.org/r/52168/#comment238514>
Don't need "This function"/"This method" etc. in javadocs.
s/constructs/creates or builds/
s/from/for.
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 121)
<https://reviews.apache.org/r/52168/#comment238515>
s/denotes//
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 131)
<https://reviews.apache.org/r/52168/#comment238423>
"_get coordinator stream_ config"
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 131)
<https://reviews.apache.org/r/52168/#comment238424>
"build coordinator stream config"
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 137)
<https://reviews.apache.org/r/52168/#comment238516>
Second sentence is unnecessary.
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 139)
<https://reviews.apache.org/r/52168/#comment238517>
s/denotes//
"the job instance to get the jobModel for"
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 151)
<https://reviews.apache.org/r/52168/#comment238426>
"system stream _consumer_" in all these messages.
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 151)
<https://reviews.apache.org/r/52168/#comment238427>
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 161)
<https://reviews.apache.org/r/52168/#comment238428>
Don't inline config in log message (it's a map), add it to the end.
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 165)
<https://reviews.apache.org/r/52168/#comment238432>
Looks like this calls coordinatorSystemProducer and Consumer's start. Same with changelogManager.start. Are they idempotent?
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 165)
<https://reviews.apache.org/r/52168/#comment238433>
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 166)
<https://reviews.apache.org/r/52168/#comment238429>
Extract variables for parameters, esp. those that have side effects.
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 169)
<https://reviews.apache.org/r/52168/#comment238434>
Don't need to stop localityManager/changelogManager?
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java (line 33)
<https://reviews.apache.org/r/52168/#comment238435>
This comment is not meaningful. Should explain what kind of TaskProxy instances instead.
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java (line 33)
<https://reviews.apache.org/r/52168/#comment238436>
First phrase is obvious, can delete. Should explain what kind of TaskProxy instances instead.
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java (line 30)
<https://reviews.apache.org/r/52168/#comment238440>
Should explain what the abstraction is instead of mentioning that it's an abstraction.
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java (line 32)
<https://reviews.apache.org/r/52168/#comment238437>
Not a meaningful comment - you're just describing what interfaces are for. Should delete.
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java (line 32)
<https://reviews.apache.org/r/52168/#comment238438>
Not sure what this means or why this is required. Should delete.
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java (line 38)
<https://reviews.apache.org/r/52168/#comment238444>
Missing description. Noticed this somewhere else too.
s/has/have
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java (line 42)
<https://reviews.apache.org/r/52168/#comment238519>
what status?
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java (line 42)
<https://reviews.apache.org/r/52168/#comment238520>
what status?
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java (lines 63 - 65)
<https://reviews.apache.org/r/52168/#comment238526>
Fix indentation over multiple lines.
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java (line 107)
<https://reviews.apache.org/r/52168/#comment238528>
Responses.serverErrorResponse
samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java (line 25)
<https://reviews.apache.org/r/52168/#comment238529>
Incorrect description - it's a lot more general than the class itself.
samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java (line 34)
<https://reviews.apache.org/r/52168/#comment238531>
Incorrect description. Should only be used for internal server errors.
Also, where's the Not Found (404) error response that we've documented? Bad request is usually a 400, not a 404. Does it mean a 404 in javax.ws?
samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java (line 44)
<https://reviews.apache.org/r/52168/#comment238532>
"Constructs a bad request (HTTP 400) response"
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java (line 40)
<https://reviews.apache.org/r/52168/#comment238534>
If this resource is always for a particular job, should "/{jobName}/{jobId}" shouldn't go here instead of specific methods? Is that not allowed in javax.ws?
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java (line 54)
<https://reviews.apache.org/r/52168/#comment238535>
This is assuming a particular implementation, what's the factory for in this case?
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java (line 58)
<https://reviews.apache.org/r/52168/#comment238418>
Missing description.
- Prateek Maheshwari
On Feb. 15, 2017, 10:26 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2017, 10:26 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Prateek Maheshwari via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review168039
-----------------------------------------------------------
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java
Lines 66 (patched)
<https://reviews.apache.org/r/52168/#comment240059>
Log and throw, or add message to SamzaException.
- Prateek Maheshwari
On Feb. 15, 2017, 10:26 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2017, 10:26 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/52168/diff/13/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Prateek Maheshwari via Review Board <no...@reviews.apache.org>.
> On March 6, 2017, 2:33 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
> > Line 121 (original), 154 (patched)
> > <https://reviews.apache.org/r/52168/diff/13/?file=1655795#file1655795line164>
> >
> > Should this be previousChangelogMapping?
>
> Shanthoosh Venkataraman wrote:
> Fixed.
Don't see this in the latest RB. Did you decide not to?
> On March 6, 2017, 2:33 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
> > Line 189 (original), 223 (patched)
> > <https://reviews.apache.org/r/52168/diff/13/?file=1655795#file1655795line234>
> >
> > The fact that this is the previousChangelogPartitionMapping seems to be significant here. If so, should call it that.
> >
> > Nitpick: Let's make changelog/changeLog usage consistent in this class.
>
> Shanthoosh Venkataraman wrote:
> Renamed to previousChangeLogPartitionMapping.
>
> Fixing the nitpick is tricky. There're classes (SystemAdmin, Config, ChangeLogManager, etc) where this name (changeLog, changelog) is used intermittently and those methods are used heavily within this class and across the codebase. If we want to pick a name it should be done consistently across the codebase. So I'm not comfortable introducing this massive change as a part of this patch. We could do it later.
Makes sense, thanks.
> On March 6, 2017, 2:33 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
> > Line 332 (original), 329 (patched)
> > <https://reviews.apache.org/r/52168/diff/13/?file=1655795#file1655795line395>
> >
> > Do you know why we pass in a server here when this class is managing (starting/stopping) it? If it's just to share the jobModelRef, should pass that directly.
>
> Shanthoosh Venkataraman wrote:
> AFAIK, this class was exposing http endpoint for reComputing the jobModel in the past(which is no longer supported). Your suggestion is correct, I can create a JIRA for improving/fixing it.
Doesn't seem like a big change, I'm OK with fixing it here. Up to you.
- Prateek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review168041
-----------------------------------------------------------
On Feb. 15, 2017, 10:26 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2017, 10:26 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/52168/diff/14/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Prateek Maheshwari via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review168041
-----------------------------------------------------------
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 50 (original), 61 (patched)
<https://reviews.apache.org/r/52168/#comment240088>
File name doesn't match class name.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 59 (original), 71 (patched)
<https://reviews.apache.org/r/52168/#comment240086>
Javadoc must contain description, here and everywhere else. Also fix @param description indentation over multiple lines.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 70 (original), 85 (patched)
<https://reviews.apache.org/r/52168/#comment240065>
"system stream consumer" etc.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 85 (original), 103 (patched)
<https://reviews.apache.org/r/52168/#comment240102>
This should be a private method or inlined. If you really want to share this code, (I'm not convinced we need to) it should not be in this class.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Lines 120 (patched)
<https://reviews.apache.org/r/52168/#comment240114>
What if this is null?
Prefer previousChangelogPartitionMapping. Verbose but more accurate.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Lines 122-134 (patched)
<https://reviews.apache.org/r/52168/#comment240118>
Prefer extracting this to a method called 'updateChangelogMapping' or something similar.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 121 (original), 154 (patched)
<https://reviews.apache.org/r/52168/#comment240091>
Should this be previousChangelogMapping?
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 185 (original), 217 (patched)
<https://reviews.apache.org/r/52168/#comment240092>
Not your change, but first sentence is unnecessary.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 186 (original), 218 (patched)
<https://reviews.apache.org/r/52168/#comment240087>
Fix indentation (-1)
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Lines 219 (patched)
<https://reviews.apache.org/r/52168/#comment240093>
Not your change, but shouldn't refer to historical context in comments - that belongs in PR/RB descriptions. I.e., "This method no longer needs" vs "This method does not need".
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 188 (original), 222 (patched)
<https://reviews.apache.org/r/52168/#comment240112>
Prefer renaming to 'readCurrentJobModel' or something similar.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 189 (original), 223 (patched)
<https://reviews.apache.org/r/52168/#comment240121>
The fact that this is the previousChangelogPartitionMapping seems to be significant here. If so, should call it that.
Nitpick: Let's make changelog/changeLog usage consistent in this class.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 235 (original)
<https://reviews.apache.org/r/52168/#comment240126>
Where is this newChangelogMapping being used after it's written with the changelog manager? Does this mean that the updated mapping is only relevant for the next job run?
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Line 332 (original), 329 (patched)
<https://reviews.apache.org/r/52168/#comment240109>
Do you know why we pass in a server here when this class is managing (starting/stopping) it? If it's just to share the jobModelRef, should pass that directly.
- Prateek Maheshwari
On Feb. 15, 2017, 10:26 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2017, 10:26 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/52168/diff/13/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/
-----------------------------------------------------------
(Updated Feb. 16, 2017, 6:26 a.m.)
Review request for samza.
Changes
-------
Changes that are related to JobCoordinator.scala:
a) This change moves the logic implemented in the initializeJobModel method to recompute & write the new changeLogPartitionMapping to coordinator stream to apply method (there by making the initializeJobModel(purely readonly) and it can be exposed via a resource). This will make initializeJobModel just a reader of jobModel [JobCoordinator.scala Lines 120-134 has the change].
b) Removes refreshJobModel method in JobCoordinator and merges it with initializeJobModel. After the current change both the refreshJobModel & initializeJobModel each contains portions of code that reads the JobModel from coordinator stream, hence the reason to merge them into a single method. Also, there are no other uses of refreshJobModel method except this, hence the felt the need to merge them into one [JobCoordinator.scala lines 198-220].
c) Moves the changlogParititonManager.start(), localityManager.start() to apply method, since both the objects are created there. I think it\u2019s better to do it over there, more like bootstrap/initializing them properly. I can\u2019t wrap my head to understand why the existing implementation had it scattered across different methods, there by making it harder to understand the code and the flow [JobCoordinator.scala Lines 95-99].
d) Extracted out the logic to build systemAdmins into a method getSystemAdmins(config), so that it can be used from the SamzaTaskProxy resource while invoking initializeJobModel to build the jobModel. This extraction might seem unnecessary, but it removes code duplication and promotes reusability in samza-rest. [JobCoordinator.scala lines 274-288].
e) Stop the coordinatorStreamProducer and coordinatorStreamConsumer in apply method in finally block. This is done since both the producer and consumer will no longer be required for the reading/updating of jobModel at the end of apply method.[JobCoordinator.scala lines 139-145].
f) Moved reWriteConfig from JobRunner.scala to Util.scala, since it's required in samza-rest to apply custom rewriters to rewrite the job config before fetching from coordinatorStream[JobRunner.scala Lines 39-61].
Apart from these, the other changes in JobCoordinator.scala are minor name changes and extracting variable that should've been constants, fixing the imports(These were done with an intent to improve readability) JobCoordinator.scala Lines (33-39, 42-51, 63, 91, 188-189).
This refactoring was done with a goal that the jobModel.read in JobCoordinator.scala is not reimplemented entirely in samza-rest.
SamzaTaskProxy #getJobModel contains the bootstrap of CoordinatorStreamSystemConsumer, CoordinatorStreamSystemProducer to read the job model from coordinator stream.
Tested the changes with a sample samza job and with samza-rest monitor.
Repository: samza
Description
-------
This patch contains the following changes
* Http get api to list the complete details of all the tasks that belongs to a job.
* Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
Diffs (updated)
-----
docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52168/diff/
Testing
-------
Manual and unit testing has been done to verify the apis.
Thanks,
Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/
-----------------------------------------------------------
(Updated Nov. 9, 2016, 1:23 a.m.)
Review request for samza.
Repository: samza
Description
-------
This patch contains the following changes
* Http get api to list the complete details of all the tasks that belongs to a job.
* Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
Diffs (updated)
-----
docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52168/diff/
Testing
-------
Manual and unit testing has been done to verify the apis.
Thanks,
Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Prateek Maheshwari <pm...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review154832
-----------------------------------------------------------
samza-core/src/main/scala/org/apache/samza/util/Util.scala (line 218)
<https://reviews.apache.org/r/52168/#comment224555>
- Prateek Maheshwari
On Nov. 3, 2016, 6:04 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Nov. 3, 2016, 6:04 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala fcabc69a829fd26b7f4e422d9877ec0364d308ce
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 103
> > <https://reviews.apache.org/r/52168/diff/10/?file=1553937#file1553937line103>
> >
> > Why do we have preferredHost here? The terminology is really confusing. Isn't `preferredHost` referring to container's host usually?
Was the result of careless refactoring. Removed.
> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, line 144
> > <https://reviews.apache.org/r/52168/diff/10/?file=1553939#file1553939line144>
> >
> > Nuke this method.
> >
> > Doesn't add any value apart from string concat. Curious about what value this adds?
Removed container name from the TasksResource. This is no longer needed. Removed.
> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 316
> > <https://reviews.apache.org/r/52168/diff/10/?file=1553940#file1553940line316>
> >
> > Reword docs.
> >
> > I'm sure there are other ways to read changelog partition mapping apart from this method.
Done.
> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 320
> > <https://reviews.apache.org/r/52168/diff/10/?file=1553940#file1553940line320>
> >
> > Not convinced we need this utility method here.
> >
> > Also, read seems to call `start` on the changeLog manager while `write` does not.
> >
> > Then, is the assumption that read must be called prior to write.?
This method is inlined to its invocation.
> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 337
> > <https://reviews.apache.org/r/52168/diff/10/?file=1553940#file1553940line337>
> >
> > This method does not add value.
> >
> > Also, it's weird that `writeChangeLogMapping` is doing a `Read-modify-write operation` on the manager.
This method is inlined to its invocation.
> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 23
> > <https://reviews.apache.org/r/52168/diff/10/?file=1553937#file1553937line23>
> >
> > Can we link this to the JobsResource? That way, it;s obvious to the reader?
Done.
> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 22
> > <https://reviews.apache.org/r/52168/diff/10/?file=1553937#file1553937line22>
> >
> > I don't think the line that `Additional operations will be added later` is meaningful. You can maybe remove it?
Done.
- Shanthoosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review154972
-----------------------------------------------------------
On Nov. 9, 2016, 1:23 a.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Nov. 9, 2016, 1:23 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Jagadish Venkatraman <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review154972
-----------------------------------------------------------
Draft review so that I don't loose comments. Overall, it's hard to convince (atleast it's not obvious to me from `diff`s) that the Job coordinator's existing functionality is not broken by this moving of methods around.
Will do need another pass of review.
docs/learn/documentation/versioned/rest/resources/tasks.md (line 22)
<https://reviews.apache.org/r/52168/#comment224779>
I don't think the line that `Additional operations will be added later` is meaningful. You can maybe remove it?
docs/learn/documentation/versioned/rest/resources/tasks.md (line 23)
<https://reviews.apache.org/r/52168/#comment224778>
Can we link this to the JobsResource? That way, it;s obvious to the reader?
docs/learn/documentation/versioned/rest/resources/tasks.md (line 103)
<https://reviews.apache.org/r/52168/#comment224780>
Why do we have preferredHost here? The terminology is really confusing. Isn't `preferredHost` referring to container's host usually?
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 143)
<https://reviews.apache.org/r/52168/#comment224781>
Nuke this method.
Doesn't add any value apart from string concat. Curious about what value this adds?
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 272)
<https://reviews.apache.org/r/52168/#comment224783>
Reword docs.
I'm sure there are other ways to read changelog partition mapping apart from this method.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 276)
<https://reviews.apache.org/r/52168/#comment224814>
Not convinced we need this utility method here.
Also, read seems to call `start` on the changeLog manager while `write` does not.
Then, is the assumption that read must be called prior to write.?
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 293)
<https://reviews.apache.org/r/52168/#comment224811>
This method does not add value.
Also, it's weird that `writeChangeLogMapping` is doing a `Read-modify-write operation` on the manager.
- Jagadish Venkatraman
On Nov. 4, 2016, 1:04 a.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2016, 1:04 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala fcabc69a829fd26b7f4e422d9877ec0364d308ce
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/
-----------------------------------------------------------
(Updated Nov. 4, 2016, 1:04 a.m.)
Review request for samza.
Repository: samza
Description
-------
This patch contains the following changes
* Http get api to list the complete details of all the tasks that belongs to a job.
* Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
Diffs (updated)
-----
docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala fcabc69a829fd26b7f4e422d9877ec0364d308ce
samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52168/diff/
Testing
-------
Manual and unit testing has been done to verify the apis.
Thanks,
Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/
-----------------------------------------------------------
(Updated Nov. 3, 2016, 11:19 p.m.)
Review request for samza.
Repository: samza
Description
-------
This patch contains the following changes
* Http get api to list the complete details of all the tasks that belongs to a job.
* Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
Diffs (updated)
-----
docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala fcabc69a829fd26b7f4e422d9877ec0364d308ce
samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52168/diff/
Testing
-------
Manual and unit testing has been done to verify the apis.
Thanks,
Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, line 104
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552722#file1552722line104>
> >
> > Don't think we need a Util method for a string concat.
Container name is used in multiple places(SamzaContainer and TasksResource). The way in which the container name is constructed from container id could change in the future, hence this was added just to encapsulate that here.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 44
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line44>
> >
> > This is an unconventional url. What's the difference b/w jobName and jobId? Why do you need two identifiers for a resource?
Each samza job is uniquely identified by a (JobName, JobId). Each upgrade of the job has different jobId and will be associated with different coordinator stream. Hence the JobModel would be different for each of them.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 55
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line55>
> >
> > What's the difference b/w containerId and containerName?
Container Name is the unique name that identifies a container within a job. Exposing this information is useful when killing containers(performing related admin actions on it.). Currently container name is generated prefixing container id with a string.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 79
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line79>
> >
> > What's a job instance? If you're referring to i001, that's LI terminology.
> >
> > "as an argument"
Job instance here means (jobId, jobName) tuple as a error message. This is used consistently in samza-rest services for this particular type of error messages.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 101
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line101>
> >
> > Last sentence sounds redundant within itself and with the sentence above. Should remove.
Done.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 105
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line105>
> >
> > Last sentence is unnecessary.
Removed.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 76
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line76>
> >
> > Don't need intermediate val - last value is always the return value in Scala. Same for other methods.
Done.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 88
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line88>
> >
> > No space b/w ) and : everywhere.
> >
> > s/retrieve/read?
Done.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 110
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line110>
> >
> > Should probably be a class val (constant).
Done.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 282
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line282>
> >
> > Don't need intermediate val.
Done.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 252
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line252>
> >
> > Feels like we're logging this in multiple places. Just make sure we're not double logging this.
Each of the logging happens in seperate control flows. It's not double logged.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java, line 40
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552739#file1552739line40>
> >
> > Is the CONFIG_ prefix a samza-rest convention?
Yes, it's the convention in samza-rest.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java, line 27
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552728#file1552728line27>
> >
> > "partition id"
Done.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 309
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line309>
> >
> > Unrelated to RB: mutable.Map instead of util.HashMap?
I would really like to make this change, however, in this class, everywhere util.Map, util.Set has been used extensively. If i just change here alone to collections.mutable.mao, it will look incoherent and changing at every other place would be a harder effort.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, lines 110-123
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line110>
> >
> > Can't tell in RB: could this be replaced by retrieveJobModel....()?
There are lot of state changes that has to be done after retrieveJobModel. LocalityManager, changeLogManager are required for that. This refactoring would make it unclean.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 98
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line98>
> >
> > Extract val for metadata cache, systemAdmins.
Done.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, lines 99-102
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line99>
> >
> > Not required for the other place they're used?
Yes.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 340
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line340>
> >
> > Previous line.
Done.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 22
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line22>
> >
> > "support operations"
Done.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 25
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line25>
> >
> > "with their"
Done.
- Shanthoosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review154675
-----------------------------------------------------------
On Nov. 4, 2016, 1:04 a.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2016, 1:04 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala fcabc69a829fd26b7f4e422d9877ec0364d308ce
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Prateek Maheshwari <pm...@linkedin.com>.
> On Nov. 2, 2016, 11:32 p.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 55
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line55>
> >
> > What's the difference b/w containerId and containerName?
>
> Shanthoosh Venkataraman wrote:
> Container Name is the unique name that identifies a container within a job. Exposing this information is useful when killing containers(performing related admin actions on it.). Currently container name is generated prefixing container id with a string.
I don't think adding a separate container name concept is necessary. The containerId is sufficient to uniquely identify the container. The "samza-container" prefix makes sense in the MDC for disambiguation in the log lines, but not sure what it buys us here.
> On Nov. 2, 2016, 11:32 p.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 79
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line79>
> >
> > What's a job instance? If you're referring to i001, that's LI terminology.
> >
> > "as an argument"
>
> Shanthoosh Venkataraman wrote:
> Job instance here means (jobId, jobName) tuple as a error message. This is used consistently in samza-rest services for this particular type of error messages.
Makes sense, thanks.
> On Nov. 2, 2016, 11:32 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, lines 110-123
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line110>
> >
> > Can't tell in RB: could this be replaced by retrieveJobModel....()?
>
> Shanthoosh Venkataraman wrote:
> There are lot of state changes that has to be done after retrieveJobModel. LocalityManager, changeLogManager are required for that. This refactoring would make it unclean.
Makes sense, thanks.
> On Nov. 2, 2016, 11:32 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 309
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line309>
> >
> > Unrelated to RB: mutable.Map instead of util.HashMap?
>
> Shanthoosh Venkataraman wrote:
> I would really like to make this change, however, in this class, everywhere util.Map, util.Set has been used extensively. If i just change here alone to collections.mutable.mao, it will look incoherent and changing at every other place would be a harder effort.
Thanks for checking, let's do it later.
> On Nov. 2, 2016, 11:32 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, lines 53-54
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line53>
> >
> > Unrelated to RB: Why does this use both? One of these is deprecated.
For context, we decided to punt on this for now since it's not a trivial refactor.
> On Nov. 2, 2016, 11:32 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, line 104
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552722#file1552722line104>
> >
> > Don't think we need a Util method for a string concat.
>
> Shanthoosh Venkataraman wrote:
> Container name is used in multiple places(SamzaContainer and TasksResource). The way in which the container name is constructed from container id could change in the future, hence this was added just to encapsulate that here.
See comment above.
> On Nov. 2, 2016, 11:32 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, lines 99-102
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line99>
> >
> > Not required for the other place they're used?
>
> Shanthoosh Venkataraman wrote:
> Yes.
"Yes, it's required" or "Yes, it's not required"? If the latter, why?
> On Nov. 2, 2016, 11:32 p.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 252
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line252>
> >
> > Feels like we're logging this in multiple places. Just make sure we're not double logging this.
>
> Shanthoosh Venkataraman wrote:
> Each of the logging happens in seperate control flows. It's not double logged.
Thanks for verifying.
- Prateek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review154675
-----------------------------------------------------------
On Nov. 3, 2016, 6:04 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Nov. 3, 2016, 6:04 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala fcabc69a829fd26b7f4e422d9877ec0364d308ce
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 55
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line55>
> >
> > What's the difference b/w containerId and containerName?
>
> Shanthoosh Venkataraman wrote:
> Container Name is the unique name that identifies a container within a job. Exposing this information is useful when killing containers(performing related admin actions on it.). Currently container name is generated prefixing container id with a string.
>
> Prateek Maheshwari wrote:
> I don't think adding a separate container name concept is necessary. The containerId is sufficient to uniquely identify the container. The "samza-container" prefix makes sense in the MDC for disambiguation in the log lines, but not sure what it buys us here.
Removed container Name from the response.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, lines 99-102
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line99>
> >
> > Not required for the other place they're used?
>
> Shanthoosh Venkataraman wrote:
> Yes.
>
> Prateek Maheshwari wrote:
> "Yes, it's required" or "Yes, it's not required"? If the latter, why?
Yes, it's required. It was added in the refactoring as well.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, line 104
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552722#file1552722line104>
> >
> > Don't think we need a Util method for a string concat.
>
> Shanthoosh Venkataraman wrote:
> Container name is used in multiple places(SamzaContainer and TasksResource). The way in which the container name is constructed from container id could change in the future, hence this was added just to encapsulate that here.
>
> Prateek Maheshwari wrote:
> See comment above.
Removed container name. Just exposing containerId.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java, line 52
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552732#file1552732line52>
> >
> > config.getConfigJobConfigFactory?
Result of careless name refactoring. Fixed.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line 75
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line75>
> >
> > Should take MetricsRegistry, not MetricsRegistryMap. Same everywhere else, here and in samza-rest.
Making this change in samza-rest in straight forward. However, doing it in Samza-core is very hard. MetricsRegistryMap in this constructor is dependent upon by the constructor of the following classes ClusterBasedJobCoordinator, ContainerProcessManager, ContainerProcessManagerMetrics. Cost associated with this change would be touching a lot of classes and their associated tests. We should punt this for now and do it later.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala, line 30
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552721#file1552721line30>
> >
> > Prefer explicit imports
Done.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 122
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line122>
> >
> > "within the installation path"
Done.
> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 116
> > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line116>
> >
> > Don't split class names: "TaskProxyFactory", "TaskProxy".
> >
> > What's the <li></li> for? Did you mean a <br/>?
> >
> > "Has support to get" -> "Gets"
> >
> > Remove "abstraction" after SimpleInstallationRecord
Done.
- Shanthoosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review154675
-----------------------------------------------------------
On Nov. 9, 2016, 1:23 a.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Nov. 9, 2016, 1:23 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Prateek Maheshwari <pm...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review154675
-----------------------------------------------------------
docs/learn/documentation/versioned/rest/resources/tasks.md (line 22)
<https://reviews.apache.org/r/52168/#comment224353>
"support operations"
docs/learn/documentation/versioned/rest/resources/tasks.md (line 25)
<https://reviews.apache.org/r/52168/#comment224309>
"with their"
docs/learn/documentation/versioned/rest/resources/tasks.md (line 44)
<https://reviews.apache.org/r/52168/#comment224311>
This is an unconventional url. What's the difference b/w jobName and jobId? Why do you need two identifiers for a resource?
docs/learn/documentation/versioned/rest/resources/tasks.md (line 55)
<https://reviews.apache.org/r/52168/#comment224356>
What's the difference b/w containerId and containerName?
docs/learn/documentation/versioned/rest/resources/tasks.md (line 79)
<https://reviews.apache.org/r/52168/#comment224313>
What's a job instance? If you're referring to i001, that's LI terminology.
"as an argument"
docs/learn/documentation/versioned/rest/resources/tasks.md (line 101)
<https://reviews.apache.org/r/52168/#comment224314>
Last sentence sounds redundant within itself and with the sentence above. Should remove.
docs/learn/documentation/versioned/rest/resources/tasks.md (line 102)
<https://reviews.apache.org/r/52168/#comment224315>
Not sure what this refers to: "customizations in the job package structure"?
docs/learn/documentation/versioned/rest/resources/tasks.md (line 105)
<https://reviews.apache.org/r/52168/#comment224316>
Last sentence is unnecessary.
docs/learn/documentation/versioned/rest/resources/tasks.md (line 116)
<https://reviews.apache.org/r/52168/#comment224317>
Don't split class names: "TaskProxyFactory", "TaskProxy".
What's the <li></li> for? Did you mean a <br/>?
"Has support to get" -> "Gets"
Remove "abstraction" after SimpleInstallationRecord
docs/learn/documentation/versioned/rest/resources/tasks.md (line 119)
<https://reviews.apache.org/r/52168/#comment224318>
Second sentence is confusing. Also not necessarily true (e.g. for network file systems).
Last sentence sounds the wrong way around. InstallationRecord implementation should conform to the directory structure. If there's a default, maybe mention or link it?
docs/learn/documentation/versioned/rest/resources/tasks.md (line 122)
<https://reviews.apache.org/r/52168/#comment224355>
"within the installation path"
samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala (line 29)
<https://reviews.apache.org/r/52168/#comment224320>
Prefer explicit imports
samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala (line 106)
<https://reviews.apache.org/r/52168/#comment224321>
TBD: This probably shouldn't be in JobConfig.
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 103)
<https://reviews.apache.org/r/52168/#comment224322>
Don't think we need a Util method for a string concat.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (lines 53 - 54)
<https://reviews.apache.org/r/52168/#comment224323>
Unrelated to RB: Why does this use both? One of these is deprecated.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 75)
<https://reviews.apache.org/r/52168/#comment224325>
Should take MetricsRegistry, not MetricsRegistryMap. Same everywhere else, here and in samza-rest.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 76)
<https://reviews.apache.org/r/52168/#comment224324>
Don't need intermediate val - last value is always the return value in Scala. Same for other methods.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 88)
<https://reviews.apache.org/r/52168/#comment224327>
No space b/w ) and : everywhere.
s/retrieve/read?
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 91)
<https://reviews.apache.org/r/52168/#comment224328>
Returning tuples is a generally a code smell. Split into 2 methods if possible.
Should this be inside the try?
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 94)
<https://reviews.apache.org/r/52168/#comment224330>
What config is this? If this is the entire config, why take it out of coordinatorSystemConsumer instead of using directly? If not, update log message to clarify what this is.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 98)
<https://reviews.apache.org/r/52168/#comment224326>
Extract val for metadata cache, systemAdmins.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (lines 99 - 102)
<https://reviews.apache.org/r/52168/#comment224340>
Not required for the other place they're used?
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 109)
<https://reviews.apache.org/r/52168/#comment224333>
Can move to previous line. Also pass MetricsRegistry.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 110)
<https://reviews.apache.org/r/52168/#comment224334>
Should probably be a class val (constant).
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (lines 110 - 123)
<https://reviews.apache.org/r/52168/#comment224357>
Can't tell in RB: could this be replaced by retrieveJobModel....()?
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 231)
<https://reviews.apache.org/r/52168/#comment224335>
Feels like we're logging this in multiple places. Just make sure we're not double logging this.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 250)
<https://reviews.apache.org/r/52168/#comment224336>
Don't need intermediate val.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 277)
<https://reviews.apache.org/r/52168/#comment224338>
Unrelated to RB: mutable.Map instead of util.HashMap?
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 307)
<https://reviews.apache.org/r/52168/#comment224339>
Previous line.
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java (line 27)
<https://reviews.apache.org/r/52168/#comment224341>
"partition id"
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java (line 51)
<https://reviews.apache.org/r/52168/#comment224344>
config.getConfigJobConfigFactory?
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java (line 40)
<https://reviews.apache.org/r/52168/#comment224349>
Is the CONFIG_ prefix a samza-rest convention?
- Prateek Maheshwari
On Nov. 2, 2016, 5:58 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Nov. 2, 2016, 5:58 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala fcabc69a829fd26b7f4e422d9877ec0364d308ce
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/
-----------------------------------------------------------
(Updated Nov. 3, 2016, 12:58 a.m.)
Review request for samza.
Repository: samza
Description
-------
This patch contains the following changes
* Http get api to list the complete details of all the tasks that belongs to a job.
* Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
Diffs (updated)
-----
docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala fcabc69a829fd26b7f4e422d9877ec0364d308ce
samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52168/diff/
Testing
-------
Manual and unit testing has been done to verify the apis.
Thanks,
Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Jagadish Venkatraman <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review153770
-----------------------------------------------------------
docs/learn/documentation/versioned/rest/resources/tasks.md (line 34)
<https://reviews.apache.org/r/52168/#comment223287>
Could we name this `errorMessage` so that it's explicit?
samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala (line 29)
<https://reviews.apache.org/r/52168/#comment223170>
nit: probably a redundant import here. I recommend against importing everything in `Util` class. We can explicitly invoke a certain method on Util instead of a blanket import on all methods.
samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala (line 112)
<https://reviews.apache.org/r/52168/#comment223179>
nit: We should be consistent with info messages. Either use format or string style concat.
Use re-writer name here.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 38)
<https://reviews.apache.org/r/52168/#comment223171>
nit: prefer to avoid blanket inputs. Is this importing all classes? Eitherways, we should be explicit about imports.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 70)
<https://reviews.apache.org/r/52168/#comment223180>
nit: Follow consistency with placement of helper functions. for example, `private` functions can be organized together.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 80)
<https://reviews.apache.org/r/52168/#comment223173>
I wonder if we should not be using `return`s in scala code here.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 101)
<https://reviews.apache.org/r/52168/#comment223196>
Prefer consistent with instantiating objects.
`
Metadatacache = new MetaDataCache(...);
initializeJobModel(config, changeLog, locality, cache);
`
This makes the line easier(?) on the eyes .
Also, prefer to have a
`val jobModel = initializeJobModel();`
`jobModel`
instead of using `return`; In Scala, if the last expression is what you want to return, then you can omit the return keyword.
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 104)
<https://reviews.apache.org/r/52168/#comment223172>
We could maybe group the apply() methods together in code. That way constructors follow each other and arguably, makes code easier to read.
samza-core/src/main/scala/org/apache/samza/util/Util.scala (line 31)
<https://reviews.apache.org/r/52168/#comment223174>
nit: Prefer no wild-card imports.
samza-core/src/main/scala/org/apache/samza/util/Util.scala (line 38)
<https://reviews.apache.org/r/52168/#comment223175>
Why does this class take a dependency on JobRunner? Did you mean to use any methods in the JobRunner?
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 59)
<https://reviews.apache.org/r/52168/#comment223181>
Why should this be static? Can't this be a per-instance variable?
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 113)
<https://reviews.apache.org/r/52168/#comment223234>
Suggest rename to `getCoordinatorSystemConfig`.
The actual config is present in the `JobModel` itself.
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 125)
<https://reviews.apache.org/r/52168/#comment223236>
Each call to `initializeJobModel` (and hence, by definition `retrieveJobModelFromCoordinatorStream` will end up writing messages to the coordinator stream.
Wouldn't this result in lots of messages in the coordinator stream ? (assuming a lot of requests for the jobModel from dashboards/ programmatic tools.)
Further, it may result in bugs (that are usually hard to track down) mangling the coordinatorStream when both the JobCoordinator and samza-rest write it at the same time.
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 126)
<https://reviews.apache.org/r/52168/#comment223233>
I recommend to scope this call with only coordinator system related configs. There's no need for `retrieveJobModelFromCoordinatorStream` to know the entire config-space of the Samza job.
samza-rest/src/main/java/org/apache/samza/rest/resources/ConfigFactoryBuilder.java (line 42)
<https://reviews.apache.org/r/52168/#comment223177>
Why do we even need this FactoryFactory? Seems to an unnecessary abstraction IMO.
How does this get used? Wondering if we can simply use a ConfigFactory.
samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java (line 29)
<https://reviews.apache.org/r/52168/#comment223194>
Prefer to use a private constructor here. It explicitly prevents `Responses` from being instantiated.
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java (line 80)
<https://reviews.apache.org/r/52168/#comment223291>
Log jobName, and jobId here. That way, it is easier when debugging.
- Jagadish Venkatraman
On Oct. 24, 2016, 10 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2016, 10 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/ConfigFactoryBuilder.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
> On Oct. 25, 2016, 9:06 p.m., Navina Ramesh wrote:
> > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala, line 47
> > <https://reviews.apache.org/r/52168/diff/7/?file=1544665#file1544665line47>
> >
> > I would think the ideal place for this method will be in Util as opposed to JobConfig. Can you please elaborate the rationale behind your choice?
It was initially added as static method in Util. However, util contains methods that does http get reqeusts, removing files, creating files, static factory methods. It looks like a place holder to hold all the things. Hence added it over here. May be into a seperate class.
- Shanthoosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review153819
-----------------------------------------------------------
On Nov. 3, 2016, 12:58 a.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Nov. 3, 2016, 12:58 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala fcabc69a829fd26b7f4e422d9877ec0364d308ce
> samza-rest/src/main/config/samza-rest.properties 7be0b47d1466d2199ae278247e8d81522fb6a91c
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review153819
-----------------------------------------------------------
I haven't read the previous comments. Hence, I may be repeating the same question asked by someone else. Please feel free to point me to your responses in such cases. Thanks!
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 101)
<https://reviews.apache.org/r/52168/#comment223285>
Don't need a return in scala
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 125)
<https://reviews.apache.org/r/52168/#comment223288>
Why can't we re-use the same metadata cache that is used when retrieving the job model?? Populating the cache on init is particularly expensive. This should be avoided, unless there is any strong motivation for different components to maintain their own cache!
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala
<https://reviews.apache.org/r/52168/#comment223289>
I would think the ideal place for this method will be in Util as opposed to JobConfig. Can you please elaborate the rationale behind your choice?
samza-core/src/main/scala/org/apache/samza/util/Util.scala (line 31)
<https://reviews.apache.org/r/52168/#comment223290>
Please use specific imports as opposed .* or ._
samza-rest/src/main/java/org/apache/samza/rest/model/Task.java (line 27)
<https://reviews.apache.org/r/52168/#comment223292>
nit: What is preferred preferredHost? :) Why not just say preferred host?
I think you are trying to use the variable name in the documentation. It doesn't necessarily improve readability though :)
samza-rest/src/main/java/org/apache/samza/rest/resources/ConfigFactoryBuilder.java (line 32)
<https://reviews.apache.org/r/52168/#comment223295>
"Builder" in the name of the class is confusing. I was expecting a Builder pattern for constructing an instance of ConfigFactory. Why not use the ClassLoaderHelper directly whereever this is used?
- Navina Ramesh
On Oct. 24, 2016, 10 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2016, 10 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/ConfigFactoryBuilder.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/
-----------------------------------------------------------
(Updated Oct. 24, 2016, 10 p.m.)
Review request for samza.
Repository: samza
Description
-------
This patch contains the following changes
* Http get api to list the complete details of all the tasks that belongs to a job.
* Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
Diffs (updated)
-----
docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/ConfigFactoryBuilder.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52168/diff/
Testing
-------
Manual and unit testing has been done to verify the apis.
Thanks,
Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/
-----------------------------------------------------------
(Updated Oct. 13, 2016, 11:57 p.m.)
Review request for samza.
Repository: samza
Description
-------
This patch contains the following changes
* Http get api to list the complete details of all the tasks that belongs to a job.
* Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
Diffs (updated)
-----
docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 13b72fae7815ddaea7ae03a24f1a426ca51613cc
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52168/diff/
Testing
-------
Manual and unit testing has been done to verify the apis.
Thanks,
Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/
-----------------------------------------------------------
(Updated Oct. 12, 2016, 10:54 p.m.)
Review request for samza.
Repository: samza
Description
-------
This patch contains the following changes
* Http get api to list the complete details of all the tasks that belongs to a job.
* Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
Diffs (updated)
-----
docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala df63b97e9d598ecd1840111ba490a723e410d089
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
samza-core/src/main/scala/org/apache/samza/util/Util.scala c4836f202f7eda1d4e71eac94fd48e46207b0316
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52168/diff/
Testing
-------
Manual and unit testing has been done to verify the apis.
Thanks,
Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/
-----------------------------------------------------------
(Updated Oct. 10, 2016, 11:10 p.m.)
Review request for samza.
Repository: samza
Description
-------
This patch contains the following changes
* Http get api to list the complete details of all the tasks that belongs to a job.
* Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
Diffs (updated)
-----
docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala ba38b5cfa4e61b5513ce38dd2be32438b62cd7ce
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
samza-core/src/main/scala/org/apache/samza/util/Util.scala 95a5aa0a23db2a890c19166b6031b2a3b96689f2
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52168/diff/
Testing
-------
Manual and unit testing has been done to verify the apis.
Thanks,
Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Jake Maes <jm...@apache.org>.
> On Oct. 10, 2016, 6:55 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java, lines 128-130
> > <https://reviews.apache.org/r/52168/diff/3/?file=1527782#file1527782line128>
> >
> > This doesn't look right.
> >
> > It doesn't seem like we should generally need to overwrite the job name and id.
> >
> > Why shouldn't this be only applying the standard rewriters as in JobRunner.rewriteConfig()?
> >
> > Is this compensating for something unique to your environment? If so, it should be pushed down to some part of the pluggable API.
>
> Shanthoosh Venkataraman wrote:
> Since job name & job id is required to retrieve job config from coordinator stream, it's unavailable in the job coordinator response. Even in samza-container startup, deployments provide necessary jobId, name configuration. Here we have jobId, jobName passed on as the api params, we could use it to populate the config. By pluggablity you mean plugging in (jobId, jobName) or the rewriters.
>>it's unavailable in the job coordinator response.
This config is coming from a file, not from the job coordinator.
>>Even in samza-container startup, deployments provide necessary jobId, name configuration. Here we have jobId, jobName passed on as the api params, we could use it to populate the config.
That sounds like a LinkedIn behavior, and is not a general problem. Anything specific to LI should be removed from this patch.
> On Oct. 10, 2016, 6:55 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java, lines 48-51
> > <https://reviews.apache.org/r/52168/diff/3/?file=1527780#file1527780line48>
> >
> > What is the point of this refactor?
> >
> > It seems to complicate the constructor signature with an unnecessary parameter (because the installationFinder can be retrieved from the config, so why pass both?) and it doesn't seem to enable any new code path.
>
> Shanthoosh Venkataraman wrote:
> Reason was InstallationFinder is a pluggable interface, creating the simple yarn proxy based upon config & SimpleInstallationFinder is not an extensible idea. If a user has his own implementation of InstallationFinder, it is harder to reuse SimpleYarnJobProxy with that implementation(users are required to use SimpleInstallationfinder). If supporting this kind of extensibility is not a requirement, I will revert back this refactoring. Please let me know your thoughts.
My point is that there's just no current need for this change. Since the APIs are generic, this change could be made in the future if users needed to independently provide a JobProxy, InstallationFinder, etc. But for now it distracts from the current change and unnecessarily complicates the constructor.
- Jake
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review152002
-----------------------------------------------------------
On Oct. 10, 2016, 11:10 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2016, 11:10 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala ba38b5cfa4e61b5513ce38dd2be32438b62cd7ce
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala 95a5aa0a23db2a890c19166b6031b2a3b96689f2
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
> On Oct. 10, 2016, 6:55 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java, lines 48-51
> > <https://reviews.apache.org/r/52168/diff/3/?file=1527780#file1527780line48>
> >
> > What is the point of this refactor?
> >
> > It seems to complicate the constructor signature with an unnecessary parameter (because the installationFinder can be retrieved from the config, so why pass both?) and it doesn't seem to enable any new code path.
Reason was InstallationFinder is a pluggable interface, creating the simple yarn proxy based upon config & SimpleInstallationFinder is not an extensible idea. If a user has his own implementation of InstallationFinder, it is harder to reuse SimpleYarnJobProxy with that implementation(users are required to use SimpleInstallationfinder). If supporting this kind of extensibility is not a requirement, I will revert back this refactoring. Please let me know your thoughts.
> On Oct. 10, 2016, 6:55 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java, lines 128-130
> > <https://reviews.apache.org/r/52168/diff/3/?file=1527782#file1527782line128>
> >
> > This doesn't look right.
> >
> > It doesn't seem like we should generally need to overwrite the job name and id.
> >
> > Why shouldn't this be only applying the standard rewriters as in JobRunner.rewriteConfig()?
> >
> > Is this compensating for something unique to your environment? If so, it should be pushed down to some part of the pluggable API.
Since job name & job id is required to retrieve job config from coordinator stream, it's unavailable in the job coordinator response. Even in samza-container startup, deployments provide necessary jobId, name configuration. Here we have jobId, jobName passed on as the api params, we could use it to populate the config. By pluggablity you mean plugging in (jobId, jobName) or the rewriters.
- Shanthoosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review152002
-----------------------------------------------------------
On Oct. 10, 2016, 11:10 p.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2016, 11:10 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala ba38b5cfa4e61b5513ce38dd2be32438b62cd7ce
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala 95a5aa0a23db2a890c19166b6031b2a3b96689f2
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Jake Maes <jm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review152002
-----------------------------------------------------------
This is a first pass and I was spot-checking. I'll need to do another one after the issues below are addressed.
docs/learn/documentation/versioned/rest/resources/tasks.md (lines 58 - 59)
<https://reviews.apache.org/r/52168/#comment220674>
The values here are odd examples and aren't very relatable. If the system is kafka, then the stream is a topic name, which doesn't typically have a number, and the partition id is just a number.
So, it's much less confusing to have an example like:
"system":"kafka"
"stream":"topic-name"
"partitionId":"0"
docs/learn/documentation/versioned/rest/resources/tasks.md (line 81)
<https://reviews.apache.org/r/52168/#comment220676>
It's better to use a real example for the job name and job id.
Since users tend to be familiar with hello-samza, it's common for documentation to reference the job name and job id from one of those jobs.
Better yet, this 404 message should match the JobsResource 404 message documentation exactly.
docs/learn/documentation/versioned/rest/resources/tasks.md (line 127)
<https://reviews.apache.org/r/52168/#comment220673>
Git doesn't like this extra newline at the end of the file.
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java (lines 46 - 48)
<https://reviews.apache.org/r/52168/#comment220712>
What is the point of this refactor?
It seems to complicate the constructor signature with an unnecessary parameter (because the installationFinder can be retrieved from the config, so why pass both?) and it doesn't seem to enable any new code path.
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java (line 25)
<https://reviews.apache.org/r/52168/#comment220718>
https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html
"So when should you use static import? Very sparingly!"
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (lines 53 - 58)
<https://reviews.apache.org/r/52168/#comment220719>
https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html
"So when should you use static import? Very sparingly!"
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (lines 128 - 130)
<https://reviews.apache.org/r/52168/#comment220713>
This doesn't look right.
It doesn't seem like we should generally need to overwrite the job name and id.
Why shouldn't this be only applying the standard rewriters as in JobRunner.rewriteConfig()?
Is this compensating for something unique to your environment? If so, it should be pushed down to some part of the pluggable API.
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java (lines 26 - 27)
<https://reviews.apache.org/r/52168/#comment220720>
https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html
"So when should you use static import? Very sparingly!"
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java (lines 40 - 41)
<https://reviews.apache.org/r/52168/#comment220721>
https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html
"So when should you use static import? Very sparingly!"
samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java (line 25)
<https://reviews.apache.org/r/52168/#comment220710>
unused import?
samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java (line 30)
<https://reviews.apache.org/r/52168/#comment220717>
https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html
"So when should you use static import? Very sparingly!"
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java (lines 36 - 37)
<https://reviews.apache.org/r/52168/#comment220722>
https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html
"So when should you use static import? Very sparingly!"
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java (lines 40 - 41)
<https://reviews.apache.org/r/52168/#comment220723>
https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html
"So when should you use static import? Very sparingly!"
samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java (lines 38 - 40)
<https://reviews.apache.org/r/52168/#comment220724>
https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html
"So when should you use static import? Very sparingly!"
samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java (line 60)
<https://reviews.apache.org/r/52168/#comment220681>
It's not sufficient to only test the happy path. The best way to enforce a consistent API is to make sure every interesting request/response is covered.
I don't see any coverage for 40X responses, for example.
See TestJobsResource for examples.
- Jake Maes
On Oct. 8, 2016, 12:12 a.m., Shanthoosh Venkataraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2016, 12:12 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> This patch contains the following changes
> * Http get api to list the complete details of all the tasks that belongs to a job.
> * Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
> docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
> samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala ba38b5cfa4e61b5513ce38dd2be32438b62cd7ce
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
> samza-core/src/main/scala/org/apache/samza/util/Util.scala 95a5aa0a23db2a890c19166b6031b2a3b96689f2
> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
> samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
> samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
> samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION
> samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
> samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
> samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52168/diff/
>
>
> Testing
> -------
>
> Manual and unit testing has been done to verify the apis.
>
>
> Thanks,
>
> Shanthoosh Venkataraman
>
>
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/
-----------------------------------------------------------
(Updated Oct. 8, 2016, 12:12 a.m.)
Review request for samza.
Repository: samza
Description
-------
This patch contains the following changes
* Http get api to list the complete details of all the tasks that belongs to a job.
* Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
Diffs (updated)
-----
docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala ba38b5cfa4e61b5513ce38dd2be32438b62cd7ce
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
samza-core/src/main/scala/org/apache/samza/util/Util.scala 95a5aa0a23db2a890c19166b6031b2a3b96689f2
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52168/diff/
Testing
-------
Manual and unit testing has been done to verify the apis.
Thanks,
Shanthoosh Venkataraman
Re: Review Request 52168: Tasks endpoint to list the complete details
of all tasks related to a job
Posted by Shanthoosh Venkataraman <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/
-----------------------------------------------------------
(Updated Sept. 23, 2016, 5:57 p.m.)
Review request for samza.
Repository: samza
Description
-------
This patch contains the following changes
* Http get api to list the complete details of all the tasks that belongs to a job.
* Refactored some methods in coordinator stream, to reuse the existing functionality of getting jobConfig from the coordinator stream.
Diffs (updated)
-----
docs/learn/documentation/versioned/rest/resource-directory.md 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef
docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala ba38b5cfa4e61b5513ce38dd2be32438b62cd7ce
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31
samza-core/src/main/scala/org/apache/samza/util/Util.scala 95a5aa0a23db2a890c19166b6031b2a3b96689f2
samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java 4d8647f3e1e650632e38b47ba5a8a2dac004f545
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java a935c98730f85f448c688a6baf2e8ddffdbb2cb4
samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java e0224c6bcf4aeaa336e5786ac472482507fcd382
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797
samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java 7db437b348ecd286185898b8f8ab0220d59da71a
samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52168/diff/
Testing
-------
Manual and unit testing has been done to verify the apis.
Thanks,
Shanthoosh Venkataraman