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