> On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, > > line 104 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552722#file1552722line104> > > > > 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 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line44> > > > > 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 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line55> > > > > 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 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line79> > > > > 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 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line101> > > > > 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 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line105> > > > > 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 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line76> > > > > 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 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line88> > > > > 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 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line110> > > > > 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 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line282> > > > > 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 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line252> > > > > 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 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552739#file1552739line40> > > > > 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 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552728#file1552728line27> > > > > "partition id" Done. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > line 309 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line309> > > > > Unrelated to RB: mutable.Map instead of util.HashMap? 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. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > lines 110-123 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line110> > > > > Can't tell in RB: could this be replaced by retrieveJobModel....()? 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. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > line 98 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line98> > > > > Extract val for metadata cache, systemAdmins. Done. > 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 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line99> > > > > Not required for the other place they're used? Yes. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > line 340 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line340> > > > > Previous line. Done. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > docs/learn/documentation/versioned/rest/resources/tasks.md, line 22 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line22> > > > > "support operations" Done. > On Nov. 3, 2016, 6:32 a.m., Prateek Maheshwari wrote: > > docs/learn/documentation/versioned/rest/resources/tasks.md, line 25 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552720#file1552720line25> > > > > "with their" Done. - Shanthoosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review154675 ----------------------------------------------------------- On Nov. 4, 2016, 1:04 a.m., Shanthoosh Venkataraman wrote: > > ----------------------------------------------------------- > 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 > ----- > > 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 > >
