----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review152002 -----------------------------------------------------------
This is a first pass and I was spot-checking. I'll need to do another one after the issues below are addressed. docs/learn/documentation/versioned/rest/resources/tasks.md (lines 58 - 59) <https://reviews.apache.org/r/52168/#comment220674> The values here are odd examples and aren't very relatable. If the system is kafka, then the stream is a topic name, which doesn't typically have a number, and the partition id is just a number. So, it's much less confusing to have an example like: "system":"kafka" "stream":"topic-name" "partitionId":"0" docs/learn/documentation/versioned/rest/resources/tasks.md (line 81) <https://reviews.apache.org/r/52168/#comment220676> It's better to use a real example for the job name and job id. Since users tend to be familiar with hello-samza, it's common for documentation to reference the job name and job id from one of those jobs. Better yet, this 404 message should match the JobsResource 404 message documentation exactly. docs/learn/documentation/versioned/rest/resources/tasks.md (line 127) <https://reviews.apache.org/r/52168/#comment220673> Git doesn't like this extra newline at the end of the file. samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java (lines 46 - 48) <https://reviews.apache.org/r/52168/#comment220712> What is the point of this refactor? It seems to complicate the constructor signature with an unnecessary parameter (because the installationFinder can be retrieved from the config, so why pass both?) and it doesn't seem to enable any new code path. samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java (line 25) <https://reviews.apache.org/r/52168/#comment220718> https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html "So when should you use static import? Very sparingly!" samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (lines 53 - 58) <https://reviews.apache.org/r/52168/#comment220719> https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html "So when should you use static import? Very sparingly!" samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (lines 128 - 130) <https://reviews.apache.org/r/52168/#comment220713> This doesn't look right. It doesn't seem like we should generally need to overwrite the job name and id. Why shouldn't this be only applying the standard rewriters as in JobRunner.rewriteConfig()? Is this compensating for something unique to your environment? If so, it should be pushed down to some part of the pluggable API. samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java (lines 26 - 27) <https://reviews.apache.org/r/52168/#comment220720> https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html "So when should you use static import? Very sparingly!" samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java (lines 40 - 41) <https://reviews.apache.org/r/52168/#comment220721> https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html "So when should you use static import? Very sparingly!" samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java (line 25) <https://reviews.apache.org/r/52168/#comment220710> unused import? samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java (line 30) <https://reviews.apache.org/r/52168/#comment220717> https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html "So when should you use static import? Very sparingly!" samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java (lines 36 - 37) <https://reviews.apache.org/r/52168/#comment220722> https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html "So when should you use static import? Very sparingly!" samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java (lines 40 - 41) <https://reviews.apache.org/r/52168/#comment220723> https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html "So when should you use static import? Very sparingly!" samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java (lines 38 - 40) <https://reviews.apache.org/r/52168/#comment220724> https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html "So when should you use static import? Very sparingly!" samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java (line 60) <https://reviews.apache.org/r/52168/#comment220681> It's not sufficient to only test the happy path. The best way to enforce a consistent API is to make sure every interesting request/response is covered. I don't see any coverage for 40X responses, for example. See TestJobsResource for examples. - Jake Maes On Oct. 8, 2016, 12:12 a.m., Shanthoosh Venkataraman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52168/ > ----------------------------------------------------------- > > (Updated Oct. 8, 2016, 12:12 a.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > This patch contains the following changes > * Http get api to list the complete details of all the tasks that belongs to > a job. > * Refactored some methods in coordinator stream, to reuse the existing > functionality of getting jobConfig from the coordinator stream. > > > Diffs > ----- > > docs/learn/documentation/versioned/rest/resource-directory.md > 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef > docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > ba38b5cfa4e61b5513ce38dd2be32438b62cd7ce > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > 022b480856483059cb9f837a08f97a718bc04c31 > samza-core/src/main/scala/org/apache/samza/util/Util.scala > 95a5aa0a23db2a890c19166b6031b2a3b96689f2 > samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java > 4d8647f3e1e650632e38b47ba5a8a2dac004f545 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java > 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java > a935c98730f85f448c688a6baf2e8ddffdbb2cb4 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java > 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java > e0224c6bcf4aeaa336e5786ac472482507fcd382 > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java > a566db598c284d69ea61af88fdc0851483d5a089 > > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java > 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 > samza-rest/src/main/java/org/apache/samza/rest/resources/ResourcesUtil.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java > 7db437b348ecd286185898b8f8ab0220d59da71a > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52168/diff/ > > > Testing > ------- > > Manual and unit testing has been done to verify the apis. > > > Thanks, > > Shanthoosh Venkataraman > >
