----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52168/#review166245 -----------------------------------------------------------
First pass, will look at JobCoordinator changes separately. docs/learn/documentation/versioned/rest/resource-directory.md (line 27) <https://reviews.apache.org/r/52168/#comment238537> I think you need a corresponding entry here. docs/learn/documentation/versioned/rest/resources/tasks.md (line 22) <https://reviews.apache.org/r/52168/#comment238497> docs/learn/documentation/versioned/rest/resources/tasks.md (line 22) <https://reviews.apache.org/r/52168/#comment238498> "endpoints" s/related to/for docs/learn/documentation/versioned/rest/resources/tasks.md (line 23) <https://reviews.apache.org/r/52168/#comment238499> s/built on top of/is a sub-resource of the/. Second half ("and is not") is redundant. docs/learn/documentation/versioned/rest/resources/tasks.md (line 41) <https://reviews.apache.org/r/52168/#comment238500> "the complete" is redundant. s/that belongs to/for docs/learn/documentation/versioned/rest/resources/tasks.md (line 74) <https://reviews.apache.org/r/52168/#comment238501> s/had completed/completed Second half is unnecessary. If you keep it, use past tense to be consistent with the first half. Also s/belong to/for docs/learn/documentation/versioned/rest/resources/tasks.md (line 80) <https://reviews.apache.org/r/52168/#comment238502> Space after colon. Let's not introduce a new convention, let's either use "jobName" or "job name". Update the place where this message is generated too. docs/learn/documentation/versioned/rest/resources/tasks.md (line 101) <https://reviews.apache.org/r/52168/#comment238419> "central point to interact". Is this for interacting with the task or just getting information about it? If the latter, should clarify it here and elsewhere. Document intention, not implementation, for interfaces. E.g., "For example, it allows getting information about all tasks for the job" vs "exposes a method to ...". docs/learn/documentation/versioned/rest/resources/tasks.md (line 116) <https://reviews.apache.org/r/52168/#comment238509> Not sure what you mean by "uses the SimpleInstallationRecord". If InstallationFinder is configurable, this shouldn't be referring to a particular installation record type. Do all InstallationFinders return a SimpleInstallationRecored? If that's the case, should remove "Simple" from the name. docs/learn/documentation/versioned/rest/resources/tasks.md (line 119) <https://reviews.apache.org/r/52168/#comment238539> The next two configs seem like JobsResource configs. If they are, shouldn't duplicate documentation. samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java (line 28) <https://reviews.apache.org/r/52168/#comment238420> Delete extra newline at end of documentation block, here and elsewhere. samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java (line 28) <https://reviews.apache.org/r/52168/#comment238421> samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java (line 86) <https://reviews.apache.org/r/52168/#comment238154> Minor: Prefer: Partition other = (Partition) o; return this.partitionId == other.partitionId && this.system == other.system && this.stream == other.stream; Same elsewhere. samza-rest/src/main/java/org/apache/samza/rest/model/Task.java (line 130) <https://reviews.apache.org/r/52168/#comment238512> storeNames? samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 57) <https://reviews.apache.org/r/52168/#comment238541> Not sure what "cluster agnostic" means here. Should document details about this particular implementation, e.g. that it gets the Task information and Config from the job coordinator stream. samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 77) <https://reviews.apache.org/r/52168/#comment238416> This class is classloading its own factory from config to create an instance of itself? Why is this required? samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 120) <https://reviews.apache.org/r/52168/#comment238514> Don't need "This function"/"This method" etc. in javadocs. s/constructs/creates or builds/ s/from/for. samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 121) <https://reviews.apache.org/r/52168/#comment238515> s/denotes// samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 131) <https://reviews.apache.org/r/52168/#comment238423> "_get coordinator stream_ config" samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 131) <https://reviews.apache.org/r/52168/#comment238424> "build coordinator stream config" samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 137) <https://reviews.apache.org/r/52168/#comment238516> Second sentence is unnecessary. samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 139) <https://reviews.apache.org/r/52168/#comment238517> s/denotes// "the job instance to get the jobModel for" samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 151) <https://reviews.apache.org/r/52168/#comment238426> "system stream _consumer_" in all these messages. samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 151) <https://reviews.apache.org/r/52168/#comment238427> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 161) <https://reviews.apache.org/r/52168/#comment238428> Don't inline config in log message (it's a map), add it to the end. samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 165) <https://reviews.apache.org/r/52168/#comment238432> Looks like this calls coordinatorSystemProducer and Consumer's start. Same with changelogManager.start. Are they idempotent? samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 165) <https://reviews.apache.org/r/52168/#comment238433> samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 166) <https://reviews.apache.org/r/52168/#comment238429> Extract variables for parameters, esp. those that have side effects. samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java (line 169) <https://reviews.apache.org/r/52168/#comment238434> Don't need to stop localityManager/changelogManager? samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java (line 33) <https://reviews.apache.org/r/52168/#comment238435> This comment is not meaningful. Should explain what kind of TaskProxy instances instead. samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxyFactory.java (line 33) <https://reviews.apache.org/r/52168/#comment238436> First phrase is obvious, can delete. Should explain what kind of TaskProxy instances instead. samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java (line 30) <https://reviews.apache.org/r/52168/#comment238440> Should explain what the abstraction is instead of mentioning that it's an abstraction. samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java (line 32) <https://reviews.apache.org/r/52168/#comment238437> Not a meaningful comment - you're just describing what interfaces are for. Should delete. samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java (line 32) <https://reviews.apache.org/r/52168/#comment238438> Not sure what this means or why this is required. Should delete. samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java (line 38) <https://reviews.apache.org/r/52168/#comment238444> Missing description. Noticed this somewhere else too. s/has/have samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java (line 42) <https://reviews.apache.org/r/52168/#comment238519> what status? samza-rest/src/main/java/org/apache/samza/rest/proxy/task/TaskProxy.java (line 42) <https://reviews.apache.org/r/52168/#comment238520> what status? samza-rest/src/main/java/org/apache/samza/rest/resources/BaseResourceConfig.java (lines 63 - 65) <https://reviews.apache.org/r/52168/#comment238526> Fix indentation over multiple lines. samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java (line 107) <https://reviews.apache.org/r/52168/#comment238528> Responses.serverErrorResponse samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java (line 25) <https://reviews.apache.org/r/52168/#comment238529> Incorrect description - it's a lot more general than the class itself. samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java (line 34) <https://reviews.apache.org/r/52168/#comment238531> Incorrect description. Should only be used for internal server errors. Also, where's the Not Found (404) error response that we've documented? Bad request is usually a 400, not a 404. Does it mean a 404 in javax.ws? samza-rest/src/main/java/org/apache/samza/rest/resources/Responses.java (line 44) <https://reviews.apache.org/r/52168/#comment238532> "Constructs a bad request (HTTP 400) response" samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java (line 40) <https://reviews.apache.org/r/52168/#comment238534> If this resource is always for a particular job, should "/{jobName}/{jobId}" shouldn't go here instead of specific methods? Is that not allowed in javax.ws? samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java (line 54) <https://reviews.apache.org/r/52168/#comment238535> This is assuming a particular implementation, what's the factory for in this case? samza-rest/src/main/java/org/apache/samza/rest/resources/TasksResource.java (line 58) <https://reviews.apache.org/r/52168/#comment238418> Missing description. - 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/ > > > Testing > ------- > > Manual and unit testing has been done to verify the apis. > > > Thanks, > > Shanthoosh Venkataraman > >
