----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review153770 -----------------------------------------------------------
docs/learn/documentation/versioned/rest/resources/tasks.md (line 34) <https://reviews.apache.org/r/52168/#comment223287> Could we name this `errorMessage` so that it's explicit? samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala (line 29) <https://reviews.apache.org/r/52168/#comment223170> nit: probably a redundant import here. I recommend against importing everything in `Util` class. We can explicitly invoke a certain method on Util instead of a blanket import on all methods. samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala (line 112) <https://reviews.apache.org/r/52168/#comment223179> nit: We should be consistent with info messages. Either use format or string style concat. Use re-writer name here. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 38) <https://reviews.apache.org/r/52168/#comment223171> nit: prefer to avoid blanket inputs. Is this importing all classes? Eitherways, we should be explicit about imports. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 70) <https://reviews.apache.org/r/52168/#comment223180> nit: Follow consistency with placement of helper functions. for example, `private` functions can be organized together. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 80) <https://reviews.apache.org/r/52168/#comment223173> I wonder if we should not be using `return`s in scala code here. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 101) <https://reviews.apache.org/r/52168/#comment223196> Prefer consistent with instantiating objects. ` Metadatacache = new MetaDataCache(...); initializeJobModel(config, changeLog, locality, cache); ` This makes the line easier(?) on the eyes . Also, prefer to have a `val jobModel = initializeJobModel();` `jobModel` instead of using `return`; In Scala, if the last expression is what you want to return, then you can omit the return keyword. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 104) <https://reviews.apache.org/r/52168/#comment223172> We could maybe group the apply() methods together in code. That way constructors follow each other and arguably, makes code easier to read. samza-core/src/main/scala/org/apache/samza/util/Util.scala (line 31) <https://reviews.apache.org/r/52168/#comment223174> nit: Prefer no wild-card imports. samza-core/src/main/scala/org/apache/samza/util/Util.scala (line 38) <https://reviews.apache.org/r/52168/#comment223175> Why does this class take a dependency on JobRunner? Did you mean to use any methods in the JobRunner? samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 59) <https://reviews.apache.org/r/52168/#comment223181> Why should this be static? Can't this be a per-instance variable? samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 113) <https://reviews.apache.org/r/52168/#comment223234> Suggest rename to `getCoordinatorSystemConfig`. The actual config is present in the `JobModel` itself. samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 125) <https://reviews.apache.org/r/52168/#comment223236> Each call to `initializeJobModel` (and hence, by definition `retrieveJobModelFromCoordinatorStream` will end up writing messages to the coordinator stream. Wouldn't this result in lots of messages in the coordinator stream ? (assuming a lot of requests for the jobModel from dashboards/ programmatic tools.) Further, it may result in bugs (that are usually hard to track down) mangling the coordinatorStream when both the JobCoordinator and samza-rest write it at the same time. samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 126) <https://reviews.apache.org/r/52168/#comment223233> I recommend to scope this call with only coordinator system related configs. There's no need for `retrieveJobModelFromCoordinatorStream` to know the entire config-space of the Samza job. samza-rest/src/main/java/org/apache/samza/rest/resources/ConfigFactoryBuilder.java (line 42) <https://reviews.apache.org/r/52168/#comment223177> Why do we even need this FactoryFactory? Seems to an unnecessary abstraction IMO. How does this get used? Wondering if we can simply use a ConfigFactory. samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java (line 29) <https://reviews.apache.org/r/52168/#comment223194> Prefer to use a private constructor here. It explicitly prevents `Responses` from being instantiated. samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java (line 80) <https://reviews.apache.org/r/52168/#comment223291> Log jobName, and jobId here. That way, it is easier when debugging. - Jagadish Venkatraman On Oct. 24, 2016, 10 p.m., Shanthoosh Venkataraman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52168/ > ----------------------------------------------------------- > > (Updated Oct. 24, 2016, 10 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > This patch contains the following changes > * Http get api to list the complete details of all the tasks that belongs to > a job. > * Refactored some methods in coordinator stream, to reuse the existing > functionality of getting jobConfig from the coordinator stream. > > > Diffs > ----- > > docs/learn/documentation/versioned/rest/resource-directory.md > 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef > docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION > samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala > 13b72fae7815ddaea7ae03a24f1a426ca51613cc > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > df63b97e9d598ecd1840111ba490a723e410d089 > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > 022b480856483059cb9f837a08f97a718bc04c31 > samza-core/src/main/scala/org/apache/samza/util/Util.scala > c4836f202f7eda1d4e71eac94fd48e46207b0316 > samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java > 4d8647f3e1e650632e38b47ba5a8a2dac004f545 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java > 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java > a935c98730f85f448c688a6baf2e8ddffdbb2cb4 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java > 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/ConfigFactoryBuilder.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java > e0224c6bcf4aeaa336e5786ac472482507fcd382 > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java > a566db598c284d69ea61af88fdc0851483d5a089 > > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java > 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 > samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java > 7db437b348ecd286185898b8f8ab0220d59da71a > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52168/diff/ > > > Testing > ------- > > Manual and unit testing has been done to verify the apis. > > > Thanks, > > Shanthoosh Venkataraman > >
