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 Wed, 09 Nov 2016 01:23:33 GMT


> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 103
> > <https://reviews.apache.org/r/52168/diff/10/?file=1553937#file1553937line103>
> >
> >     Why do we have preferredHost here? The terminology is really confusing. Isn't
`preferredHost` referring to container's host usually?

Was the result of careless refactoring. Removed.


> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, line
144
> > <https://reviews.apache.org/r/52168/diff/10/?file=1553939#file1553939line144>
> >
> >     Nuke this method. 
> >     
> >     Doesn't add any value apart from string concat. Curious about what value this
adds?

Removed container name from the TasksResource. This is no longer needed. Removed.


> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line
316
> > <https://reviews.apache.org/r/52168/diff/10/?file=1553940#file1553940line316>
> >
> >     Reword docs.
> >     
> >     I'm sure there are other ways to read changelog partition mapping apart from
this method.

Done.


> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line
320
> > <https://reviews.apache.org/r/52168/diff/10/?file=1553940#file1553940line320>
> >
> >     Not convinced we need this utility method here. 
> >     
> >     Also, read seems to call `start` on the changeLog manager while `write` does
not. 
> >     
> >     Then, is the assumption that read must be called prior to write.?

This method is inlined to its invocation.


> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, line
337
> > <https://reviews.apache.org/r/52168/diff/10/?file=1553940#file1553940line337>
> >
> >     This method does not add value. 
> >     
> >     Also, it's weird that `writeChangeLogMapping` is doing a `Read-modify-write
operation` on the manager.

This method is inlined to its invocation.


> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 23
> > <https://reviews.apache.org/r/52168/diff/10/?file=1553937#file1553937line23>
> >
> >     Can we link this to the JobsResource? That way, it;s obvious to the reader?

Done.


> On Nov. 4, 2016, 8:52 p.m., Jagadish Venkatraman wrote:
> > docs/learn/documentation/versioned/rest/resources/tasks.md, line 22
> > <https://reviews.apache.org/r/52168/diff/10/?file=1553937#file1553937line22>
> >
> >     I don't think the line that `Additional operations will be added later` is meaningful.
You can maybe remove it?

Done.


- Shanthoosh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52168/#review154972
-----------------------------------------------------------


On Nov. 9, 2016, 1:23 a.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
> 
> (Updated Nov. 9, 2016, 1:23 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/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/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