> On Nov. 2, 2016, 11:32 p.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? > > Shanthoosh Venkataraman wrote: > 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.
I don't think adding a separate container name concept is necessary. The containerId is sufficient to uniquely identify the container. The "samza-container" prefix makes sense in the MDC for disambiguation in the log lines, but not sure what it buys us here. > On Nov. 2, 2016, 11:32 p.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" > > Shanthoosh Venkataraman wrote: > 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. Makes sense, thanks. > On Nov. 2, 2016, 11:32 p.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....()? > > Shanthoosh Venkataraman wrote: > 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. Makes sense, thanks. > On Nov. 2, 2016, 11:32 p.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? > > Shanthoosh Venkataraman wrote: > 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. Thanks for checking, let's do it later. > On Nov. 2, 2016, 11:32 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > lines 53-54 > > <https://reviews.apache.org/r/52168/diff/8/?file=1552723#file1552723line53> > > > > Unrelated to RB: Why does this use both? One of these is deprecated. For context, we decided to punt on this for now since it's not a trivial refactor. > On Nov. 2, 2016, 11:32 p.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. > > Shanthoosh Venkataraman wrote: > 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. See comment above. > On Nov. 2, 2016, 11:32 p.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? > > Shanthoosh Venkataraman wrote: > Yes. "Yes, it's required" or "Yes, it's not required"? If the latter, why? > On Nov. 2, 2016, 11:32 p.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. > > Shanthoosh Venkataraman wrote: > Each of the logging happens in seperate control flows. It's not double > logged. Thanks for verifying. - Prateek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review154675 ----------------------------------------------------------- On Nov. 3, 2016, 6:04 p.m., Shanthoosh Venkataraman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52168/ > ----------------------------------------------------------- > > (Updated Nov. 3, 2016, 6:04 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 > >
