----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review168041 -----------------------------------------------------------
samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala Line 50 (original), 61 (patched) <https://reviews.apache.org/r/52168/#comment240088> File name doesn't match class name. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala Line 59 (original), 71 (patched) <https://reviews.apache.org/r/52168/#comment240086> Javadoc must contain description, here and everywhere else. Also fix @param description indentation over multiple lines. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala Line 70 (original), 85 (patched) <https://reviews.apache.org/r/52168/#comment240065> "system stream consumer" etc. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala Line 85 (original), 103 (patched) <https://reviews.apache.org/r/52168/#comment240102> This should be a private method or inlined. If you really want to share this code, (I'm not convinced we need to) it should not be in this class. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala Lines 120 (patched) <https://reviews.apache.org/r/52168/#comment240114> What if this is null? Prefer previousChangelogPartitionMapping. Verbose but more accurate. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala Lines 122-134 (patched) <https://reviews.apache.org/r/52168/#comment240118> Prefer extracting this to a method called 'updateChangelogMapping' or something similar. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala Line 121 (original), 154 (patched) <https://reviews.apache.org/r/52168/#comment240091> Should this be previousChangelogMapping? samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala Line 185 (original), 217 (patched) <https://reviews.apache.org/r/52168/#comment240092> Not your change, but first sentence is unnecessary. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala Line 186 (original), 218 (patched) <https://reviews.apache.org/r/52168/#comment240087> Fix indentation (-1) samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala Lines 219 (patched) <https://reviews.apache.org/r/52168/#comment240093> Not your change, but shouldn't refer to historical context in comments - that belongs in PR/RB descriptions. I.e., "This method no longer needs" vs "This method does not need". samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala Line 188 (original), 222 (patched) <https://reviews.apache.org/r/52168/#comment240112> Prefer renaming to 'readCurrentJobModel' or something similar. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala Line 189 (original), 223 (patched) <https://reviews.apache.org/r/52168/#comment240121> The fact that this is the previousChangelogPartitionMapping seems to be significant here. If so, should call it that. Nitpick: Let's make changelog/changeLog usage consistent in this class. samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala Line 235 (original) <https://reviews.apache.org/r/52168/#comment240126> Where is this newChangelogMapping being used after it's written with the changelog manager? Does this mean that the updated mapping is only relevant for the next job run? samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala Line 332 (original), 329 (patched) <https://reviews.apache.org/r/52168/#comment240109> Do you know why we pass in a server here when this class is managing (starting/stopping) it? If it's just to share the jobModelRef, should pass that directly. - Prateek Maheshwari On Feb. 15, 2017, 10:26 p.m., Shanthoosh Venkataraman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52168/ > ----------------------------------------------------------- > > (Updated Feb. 15, 2017, 10:26 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/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/13/ > > > Testing > ------- > > Manual and unit testing has been done to verify the apis. > > > Thanks, > > Shanthoosh Venkataraman > >
