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 Thu, 16 Feb 2017 06:26:17 GMT

-----------------------------------------------------------
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’s better to do it over there, more like bootstrap/initializing
them properly. I can’t 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


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