You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@samza.apache.org by Shanthoosh Venkataraman <sa...@gmail.com> on 2017/02/16 06:26:17 UTC

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

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

(Updated Feb. 16, 2017, 6:26 a.m.)


Review request for samza.


Changes
-------

Changes that are related to JobCoordinator.scala:

a) This change moves the logic implemented in the initializeJobModel method to recompute & write the new changeLogPartitionMapping to coordinator stream  to apply method (there by making the initializeJobModel(purely readonly) and it can be exposed via a resource). This will make initializeJobModel just a reader of jobModel [JobCoordinator.scala Lines 120-134 has the change].

b) Removes refreshJobModel method in JobCoordinator and merges it with initializeJobModel. After the current change both the refreshJobModel & initializeJobModel each contains portions of code that reads the JobModel from coordinator stream, hence the reason to merge them into a single method. Also, there are no other uses of refreshJobModel method except this, hence the felt the need to merge them into one [JobCoordinator.scala lines 198-220].

c) Moves the changlogParititonManager.start(), localityManager.start() to apply method, since both the objects are created there. I think it\u2019s better to do it over there, more like bootstrap/initializing them properly. I can\u2019t wrap my head to understand why the existing implementation had it scattered across different methods, there by making it harder to understand the code and the flow [JobCoordinator.scala Lines 95-99].

d) Extracted out the logic to build systemAdmins into a method getSystemAdmins(config), so that it can be used from the SamzaTaskProxy resource while invoking initializeJobModel to build the jobModel. This extraction might seem unnecessary, but it removes code duplication and promotes reusability in samza-rest. [JobCoordinator.scala lines 274-288].

e) Stop the coordinatorStreamProducer and coordinatorStreamConsumer in apply method in finally block. This is done since both the producer and consumer will no longer be required for the reading/updating of jobModel at the end of apply method.[JobCoordinator.scala lines 139-145].

f) Moved reWriteConfig from JobRunner.scala to Util.scala, since it's required in samza-rest to apply custom rewriters to rewrite the job config before fetching from coordinatorStream[JobRunner.scala Lines 39-61].

Apart from these, the other changes in JobCoordinator.scala are minor name changes and extracting variable that should've been constants, fixing the imports(These were done with an intent to improve readability) JobCoordinator.scala Lines (33-39, 42-51, 63, 91, 188-189). 

This refactoring was done with a goal that the jobModel.read in JobCoordinator.scala is not reimplemented entirely in samza-rest.

SamzaTaskProxy #getJobModel contains the bootstrap of CoordinatorStreamSystemConsumer, CoordinatorStreamSystemProducer to read the job model from coordinator stream. 

Tested the changes with a sample samza job and with samza-rest monitor.


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 (updated)
-----

  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

Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job

Posted by Xinyu Liu <xi...@gmail.com>.

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


Ship it!




Ship It!

- Xinyu Liu


On Feb. 16, 2017, 6:26 a.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
> 
> (Updated Feb. 16, 2017, 6:26 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/java/org/apache/samza/standalone/StandaloneJobCoordinator.java 46dbf30eb37f7d617fb868fb4e1561fef18522d5 
>   samza-core/src/main/java/org/apache/samza/zk/ZkJobCoordinator.java PRE-CREATION 
>   samza-core/src/main/scala/org/apache/samza/coordinator/JobModelManager.scala 14d5dff00758c6e35cae018b4ebaa686d67bb57d 
>   samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31 
>   samza-core/src/main/scala/org/apache/samza/util/Util.scala 9019d02bc83e0e76a1b6a6fb9958733dfe8b86a4 
>   samza-core/src/test/java/org/apache/samza/execution/TestExecutionPlanner.java PRE-CREATION 
>   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/15/
> 
> 
> Testing
> -------
> 
> Manual and unit testing has been done to verify the apis.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>