subject:"Re\: Review Request 52168\: Tasks endpoint to list the complete details of all tasks related to a job"

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2017-03-16 Thread Xinyu Liu


---
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

2017-03-15 Thread Xinyu Liu


---
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)


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)


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)


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/
>

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2017-03-10 Thread Prateek Maheshwari via Review Board


---
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)


"Reads the "



samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala
Lines 221 (patched)


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

2017-03-10 Thread Prateek Maheshwari via Review Board



> 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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 
>   
>

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2017-03-06 Thread Prateek Maheshwari via Review Board



> On Feb. 23, 2017, 12:24 p.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md
> > Lines 119 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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 
>

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2017-03-06 Thread Prateek Maheshwari via Review Board


---
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)


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

2017-02-23 Thread Prateek Maheshwari


---
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)


I think you need a corresponding entry here.



docs/learn/documentation/versioned/rest/resources/tasks.md (line 22)






docs/learn/documentation/versioned/rest/resources/tasks.md (line 22)


"endpoints"

s/related to/for



docs/learn/documentation/versioned/rest/resources/tasks.md (line 23)


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)


"the complete" is redundant.
s/that belongs to/for



docs/learn/documentation/versioned/rest/resources/tasks.md (line 74)


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)


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)


"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)


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)


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)


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)






samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java (line 86)


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)


storeNames?



samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java 
(line 57)


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)


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)


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)


s/denotes//



samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java 
(line 131)


"_get coordinator stream_ config"

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2016-11-08 Thread Shanthoosh Venkataraman



> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 55
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > "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
> > 
> >
> > Don't split class names: "TaskProxyFactory", "TaskProxy".
> > 
> > What's the  for? Did you mean a ?
> > 
> > "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

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2016-11-08 Thread Shanthoosh Venkataraman



> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 103
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 
>   
>

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2016-11-08 Thread Shanthoosh Venkataraman


---
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

2016-11-04 Thread Prateek Maheshwari



> On Nov. 2, 2016, 11:32 p.m., Prateek Maheshwari wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 55
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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:
>

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2016-11-03 Thread Shanthoosh Venkataraman



> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote:
> > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, 
> > line 104
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > "partition id"

Done.

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2016-11-03 Thread Shanthoosh Venkataraman


---
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

2016-11-03 Thread Shanthoosh Venkataraman


---
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

2016-11-03 Thread Prateek Maheshwari


---
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)


"support operations"



docs/learn/documentation/versioned/rest/resources/tasks.md (line 25)


"with their"



docs/learn/documentation/versioned/rest/resources/tasks.md (line 44)


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)


What's the difference b/w containerId and containerName?



docs/learn/documentation/versioned/rest/resources/tasks.md (line 79)


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)


Last sentence sounds redundant within itself and with the sentence above. 
Should remove.



docs/learn/documentation/versioned/rest/resources/tasks.md (line 102)


Not sure what this refers to: "customizations in the job package structure"?



docs/learn/documentation/versioned/rest/resources/tasks.md (line 105)


Last sentence is unnecessary.



docs/learn/documentation/versioned/rest/resources/tasks.md (line 116)


Don't split class names: "TaskProxyFactory", "TaskProxy".

What's the  for? Did you mean a ?

"Has support to get" -> "Gets"

Remove "abstraction" after SimpleInstallationRecord



docs/learn/documentation/versioned/rest/resources/tasks.md (line 119)


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)


"within the installation path"



samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala (line 29)


Prefer explicit imports



samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala (line 106)


TBD: This probably shouldn't be in JobConfig.



samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 
103)


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)


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)


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)


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)


No space b/w ) and : everywhere.

s/retrieve/read?



samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
(line 91)


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)


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)


Extract val for metadata cache, systemAdmins.

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2016-10-25 Thread Jagadish Venkatraman


---
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)


Could we name this `errorMessage` so that it's explicit?



samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala (line 29)


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)


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)


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)


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)


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)


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)


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)


nit: Prefer no wild-card imports.



samza-core/src/main/scala/org/apache/samza/util/Util.scala (line 38)


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)


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)


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)


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)


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)


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)


Prefer to use a private

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2016-10-25 Thread Navina Ramesh


---
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)


Don't need a return in scala



samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
(line 125)


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 


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)


Please use specific imports as opposed .* or ._



samza-rest/src/main/java/org/apache/samza/rest/model/Task.java (line 27)


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)


"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 
>   
>

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2016-10-24 Thread Jake Maes



> 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
> > 
> >
> > 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 
>   
>

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2016-10-24 Thread Shanthoosh Venkataraman


---
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

2016-10-17 Thread Jake Maes


---
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)


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

2016-10-17 Thread Jake Maes


---
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)


*used*



docs/learn/documentation/versioned/rest/resources/tasks.md (line 105)


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)


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)


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
>

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2016-10-17 Thread Shanthoosh Venkataraman



> On Oct. 13, 2016, 12:55 a.m., Jake Maes wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 55
> > 
> >
> > 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 
>   
>

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2016-10-17 Thread Jake Maes



> On Oct. 13, 2016, 12:55 a.m., Jake Maes wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 55
> > 
> >
> > 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

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2016-10-13 Thread Shanthoosh Venkataraman



> On Oct. 13, 2016, 12:55 a.m., Jake Maes wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 55
> > 
> >
> > 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

2016-10-12 Thread Jake Maes


---
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)


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)


For clarity, I think this property should be "preferredHost"



docs/learn/documentation/versioned/rest/resources/tasks.md (line 55)


What is the value of the container name?



docs/learn/documentation/versioned/rest/resources/tasks.md (line 96)


Does this line belong under the Configuration section?



samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala 
(line 125)


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 


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)


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)


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)


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)


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

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2016-10-12 Thread Shanthoosh Venkataraman


---
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

2016-10-12 Thread Shanthoosh Venkataraman


---
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)


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

2016-10-10 Thread Jake Maes


---
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)


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)


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)


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)


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://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://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)


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://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://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)


unused import?



samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java 
(line 30)



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://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://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://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html
"So when should you use static import? Very sparingly!"

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

2016-10-07 Thread Shanthoosh Venkataraman


---
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

2016-09-23 Thread Shanthoosh Venkataraman


---
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

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

30 matches

Site Navigation

Mail list logo

Footer information