samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Prateek Maheshwari <pmaheshw...@linkedin.com>
Subject Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
Date Thu, 23 Feb 2017 20:24:46 GMT

-----------------------------------------------------------
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
> 
>


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