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