samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Shanthoosh Venkataraman <santhoshvenkat1...@gmail.com>
Subject Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
Date Fri, 04 Nov 2016 01:06:43 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message