samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jake Maes <jma...@apache.org>
Subject Re: Review Request 52168: Tasks endpoint to list the complete details of all tasks related to a job
Date Mon, 10 Oct 2016 23:54:57 GMT


> On Oct. 10, 2016, 6:55 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/proxy/task/SamzaTaskProxy.java, lines
128-130
> > <https://reviews.apache.org/r/52168/diff/3/?file=1527782#file1527782line128>
> >
> >     This doesn't look right. 
> >     
> >     It doesn't seem like we should generally need to overwrite the job name and
id. 
> >     
> >     Why shouldn't this be only applying the standard rewriters as in JobRunner.rewriteConfig()?
> >     
> >     Is this compensating for something unique to your environment? If so, it should
be pushed down to some part of the pluggable API.
> 
> Shanthoosh Venkataraman wrote:
>     Since job name & job id is required to retrieve job config from coordinator stream,
it's unavailable in the job coordinator response. Even in samza-container startup, deployments
provide necessary jobId, name configuration. Here we have jobId, jobName passed on as the
api params, we could use it to populate the config. By pluggablity you mean plugging in (jobId,
jobName) or the rewriters.

>>it's unavailable in the job coordinator response.

This config is coming from a file, not from the job coordinator. 

>>Even in samza-container startup, deployments provide necessary jobId, name configuration.
Here we have jobId, jobName passed on as the api params, we could use it to populate the config.

That sounds like a LinkedIn behavior, and is not a general problem. Anything specific to LI
should be removed from this patch.


> On Oct. 10, 2016, 6:55 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/rest/proxy/job/SimpleYarnJobProxy.java,
lines 48-51
> > <https://reviews.apache.org/r/52168/diff/3/?file=1527780#file1527780line48>
> >
> >     What is the point of this refactor?
> >     
> >     It seems to complicate the constructor signature with an unnecessary parameter
(because the installationFinder can be retrieved from the config, so why pass both?) and it
doesn't seem to enable any new code path.
> 
> Shanthoosh Venkataraman wrote:
>     Reason was InstallationFinder is a pluggable interface, creating the simple yarn
proxy based upon config & SimpleInstallationFinder is not an extensible idea. If a user
has his own implementation of InstallationFinder, it is harder to reuse SimpleYarnJobProxy
with that implementation(users are required to use SimpleInstallationfinder). If supporting
this kind of extensibility is not a requirement, I will revert back this refactoring. Please
let me know your thoughts.

My point is that there's just no current need for this change. Since the APIs are generic,
this change could be made in the future if users needed to independently provide a JobProxy,
InstallationFinder, etc. But for now it distracts from the current change and unnecessarily
complicates the constructor.


- Jake


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


On Oct. 10, 2016, 11:10 p.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52168/
> -----------------------------------------------------------
> 
> (Updated Oct. 10, 2016, 11:10 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/container/SamzaContainer.scala 05a996c98075ea8ed3767af666b9beeb1933f2a6

>   samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala ba38b5cfa4e61b5513ce38dd2be32438b62cd7ce

>   samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 022b480856483059cb9f837a08f97a718bc04c31

>   samza-core/src/main/scala/org/apache/samza/util/Util.scala 95a5aa0a23db2a890c19166b6031b2a3b96689f2

>   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/ResourcesUtil.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/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