----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review154675 -----------------------------------------------------------
docs/learn/documentation/versioned/rest/resources/tasks.md (line 22) <https://reviews.apache.org/r/52168/#comment224353> "support operations" docs/learn/documentation/versioned/rest/resources/tasks.md (line 25) <https://reviews.apache.org/r/52168/#comment224309> "with their" docs/learn/documentation/versioned/rest/resources/tasks.md (line 44) <https://reviews.apache.org/r/52168/#comment224311> This is an unconventional url. What's the difference b/w jobName and jobId? Why do you need two identifiers for a resource? docs/learn/documentation/versioned/rest/resources/tasks.md (line 55) <https://reviews.apache.org/r/52168/#comment224356> What's the difference b/w containerId and containerName? docs/learn/documentation/versioned/rest/resources/tasks.md (line 79) <https://reviews.apache.org/r/52168/#comment224313> What's a job instance? If you're referring to i001, that's LI terminology. "as an argument" docs/learn/documentation/versioned/rest/resources/tasks.md (line 101) <https://reviews.apache.org/r/52168/#comment224314> Last sentence sounds redundant within itself and with the sentence above. Should remove. docs/learn/documentation/versioned/rest/resources/tasks.md (line 102) <https://reviews.apache.org/r/52168/#comment224315> Not sure what this refers to: "customizations in the job package structure"? docs/learn/documentation/versioned/rest/resources/tasks.md (line 105) <https://reviews.apache.org/r/52168/#comment224316> Last sentence is unnecessary. docs/learn/documentation/versioned/rest/resources/tasks.md (line 116) <https://reviews.apache.org/r/52168/#comment224317> Don't split class names: "TaskProxyFactory", "TaskProxy". What's the <li></li> for? Did you mean a <br/>? "Has support to get" -> "Gets" Remove "abstraction" after SimpleInstallationRecord docs/learn/documentation/versioned/rest/resources/tasks.md (line 119) <https://reviews.apache.org/r/52168/#comment224318> Second sentence is confusing. Also not necessarily true (e.g. for network file systems). Last sentence sounds the wrong way around. InstallationRecord implementation should conform to the directory structure. If there's a default, maybe mention or link it? docs/learn/documentation/versioned/rest/resources/tasks.md (line 122) <https://reviews.apache.org/r/52168/#comment224355> "within the installation path" samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala (line 29) <https://reviews.apache.org/r/52168/#comment224320> Prefer explicit imports samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala (line 106) <https://reviews.apache.org/r/52168/#comment224321> TBD: This probably shouldn't be in JobConfig. samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 103) <https://reviews.apache.org/r/52168/#comment224322> Don't think we need a Util method for a string concat. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (lines 53 - 54) <https://reviews.apache.org/r/52168/#comment224323> Unrelated to RB: Why does this use both? One of these is deprecated. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 75) <https://reviews.apache.org/r/52168/#comment224325> Should take MetricsRegistry, not MetricsRegistryMap. Same everywhere else, here and in samza-rest. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 76) <https://reviews.apache.org/r/52168/#comment224324> Don't need intermediate val - last value is always the return value in Scala. Same for other methods. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 88) <https://reviews.apache.org/r/52168/#comment224327> No space b/w ) and : everywhere. s/retrieve/read? samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 91) <https://reviews.apache.org/r/52168/#comment224328> Returning tuples is a generally a code smell. Split into 2 methods if possible. Should this be inside the try? samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 94) <https://reviews.apache.org/r/52168/#comment224330> What config is this? If this is the entire config, why take it out of coordinatorSystemConsumer instead of using directly? If not, update log message to clarify what this is. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 98) <https://reviews.apache.org/r/52168/#comment224326> Extract val for metadata cache, systemAdmins. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (lines 99 - 102) <https://reviews.apache.org/r/52168/#comment224340> Not required for the other place they're used? samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 109) <https://reviews.apache.org/r/52168/#comment224333> Can move to previous line. Also pass MetricsRegistry. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 110) <https://reviews.apache.org/r/52168/#comment224334> Should probably be a class val (constant). samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (lines 110 - 123) <https://reviews.apache.org/r/52168/#comment224357> Can't tell in RB: could this be replaced by retrieveJobModel....()? samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 231) <https://reviews.apache.org/r/52168/#comment224335> Feels like we're logging this in multiple places. Just make sure we're not double logging this. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 250) <https://reviews.apache.org/r/52168/#comment224336> Don't need intermediate val. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 277) <https://reviews.apache.org/r/52168/#comment224338> Unrelated to RB: mutable.Map instead of util.HashMap? samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 307) <https://reviews.apache.org/r/52168/#comment224339> Previous line. samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java (line 27) <https://reviews.apache.org/r/52168/#comment224341> "partition id" samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java (line 51) <https://reviews.apache.org/r/52168/#comment224344> config.getConfigJobConfigFactory? samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java (line 40) <https://reviews.apache.org/r/52168/#comment224349> Is the CONFIG_ prefix a samza-rest convention? - Prateek Maheshwari On Nov. 2, 2016, 5:58 p.m., Shanthoosh Venkataraman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52168/ > ----------------------------------------------------------- > > (Updated Nov. 2, 2016, 5:58 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > This patch contains the following changes > * Http get api to list the complete details of all the tasks that belongs to > a job. > * Refactored some methods in coordinator stream, to reuse the existing > functionality of getting jobConfig from the coordinator stream. > > > Diffs > ----- > > docs/learn/documentation/versioned/rest/resource-directory.md > 79746d1e2eb3491e4bd26c3c7cf6c7efd150d8ef > docs/learn/documentation/versioned/rest/resources/tasks.md PRE-CREATION > samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala > 13b72fae7815ddaea7ae03a24f1a426ca51613cc > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 05a996c98075ea8ed3767af666b9beeb1933f2a6 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > df63b97e9d598ecd1840111ba490a723e410d089 > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > 022b480856483059cb9f837a08f97a718bc04c31 > samza-core/src/main/scala/org/apache/samza/util/Util.scala > c4836f202f7eda1d4e71eac94fd48e46207b0316 > > samza-core/src/test/scala/org/apache/samza/coordinator/TestJobCoordinator.scala > fcabc69a829fd26b7f4e422d9877ec0364d308ce > samza-rest/src/main/config/samza-rest.properties > 7be0b47d1466d2199ae278247e8d81522fb6a91c > samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/AbstractJobProxy.java > 4d8647f3e1e650632e38b47ba5a8a2dac004f545 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/JobProxyFactory.java > 067711a74e5b0d7277a9c8b2d2517b56e9cfbcca > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java > a935c98730f85f448c688a6baf2e8ddffdbb2cb4 > > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxyFactory.java > 11d93d4608d23a4e3fb3bfc50dfac35ab6dbdf3c > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxyFactory.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java > PRE-CREATION > > samza-rest/src/main/java/org/apache/samza/rest/resources/DefaultResourceFactory.java > e0224c6bcf4aeaa336e5786ac472482507fcd382 > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java > a566db598c284d69ea61af88fdc0851483d5a089 > > samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResourceConfig.java > 527482d2ee55747e7b3f9c54c8a3b1afe7ad8797 > samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java > PRE-CREATION > samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestJobsResource.java > 7db437b348ecd286185898b8f8ab0220d59da71a > > samza-rest/src/test/java/org/apache/samza/rest/resources/TestTasksResource.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockInstallationFinder.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockResourceFactory.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxy.java > PRE-CREATION > > samza-rest/src/test/java/org/apache/samza/rest/resources/mock/MockTaskProxyFactory.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52168/diff/ > > > Testing > ------- > > Manual and unit testing has been done to verify the apis. > > > Thanks, > > Shanthoosh Venkataraman > >
