----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review152418 -----------------------------------------------------------
docs/learn/documentation/versioned/rest/resources/tasks.md (line 24) <https://reviews.apache.org/r/52168/#comment221427> Might be worth mentioning that this Resource builds on the JobsResource and is not intended to be used independently. docs/learn/documentation/versioned/rest/resources/tasks.md (line 52) <https://reviews.apache.org/r/52168/#comment221441> For clarity, I think this property should be "preferredHost" docs/learn/documentation/versioned/rest/resources/tasks.md (line 55) <https://reviews.apache.org/r/52168/#comment221422> What is the value of the container name? docs/learn/documentation/versioned/rest/resources/tasks.md (line 96) <https://reviews.apache.org/r/52168/#comment221426> Does this line belong under the Configuration section? samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 125) <https://reviews.apache.org/r/52168/#comment221433> nit: Between here and line 101, we're now creating 2 instances of StreamMetadataCache where we originally had only 1. This probably won't cause problems since the TTL is 0, but it's not ideal, particularly if the TTL is ever increased. Ignore this if it's difficult to change. Just noting it for the sake of the other reviewers. samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala <https://reviews.apache.org/r/52168/#comment221425> Lets not move this to Util. How about JobConfig or a funtional class called Rewriters samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java (line 40) <https://reviews.apache.org/r/52168/#comment221442> Since this is a new factory, maybe we should learn from the shortcoming of the JobProxyFactory and actually pass the MetricsRegistryMap in for this one. That'll allow us to add metrics to the TaskProxy later. samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java (line 157) <https://reviews.apache.org/r/52168/#comment221443> This is nice! Just a nit: Instead of "ResourceUtil" -> something like "ResponseTemplates" or "Responses" samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java (line 54) <https://reviews.apache.org/r/52168/#comment221444> nit: It's confusing to have job installation path in as a property of the TaskResourceConfig. Shouldn't it be BaseResourceConfig.CONFIG_JOB_INSTALLATIONS_PATH? There are multiple occurrences of this. samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java (line 55) <https://reviews.apache.org/r/52168/#comment221445> Why should this be necessary for the jobs resource tests? - Jake Maes On Oct. 12, 2016, 10:54 p.m., Shanthoosh Venkataraman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52168/ > ----------------------------------------------------------- > > (Updated Oct. 12, 2016, 10:54 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > This patch contains the following changes > * Http get api to list the complete details of all the tasks that belongs to > a job. > * Refactored some methods in coordinator stream, to reuse the existing > functionality of getting jobConfig from the coordinator stream. > > > Diffs > ----- > > docs/learn/documentation/versioned/rest/resource-directory.md > 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef > docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > df63b97e9d598ecd1840111ba490a723e410d089 > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > 022b480856483059cb9f837a08f97a718bc04c31 > samza-core/src/main/scala/org/apache/samza/util/Util.scala > c4836f202f7eda1d4e71eac94fd48e46207b0316 > samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java > 4d8647f3e1e650632e38b47ba5a8a2dac004f545 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java > 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java > a935c98730f85f448c688a6baf2e8ddffdbb2cb4 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java > 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java > e0224c6bcf4aeaa336e5786ac472482507fcd382 > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java > a566db598c284d69ea61af88fdc0851483d5a089 > > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java > 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 > samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java > 7db437b348ecd286185898b8f8ab0220d59da71a > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52168/diff/ > > > Testing > ------- > > Manual and unit testing has been done to verify the apis. > > > Thanks, > > Shanthoosh Venkataraman > >
