> On Nov. 10, 2015, 4:07 p.m., Jarek Cecho wrote: > > Since now we don't have ability to specify the job ID as the way to execute > > the job, might I suggest to add a test case to our integration test suite > > that will create link/job name with: spaces, "." and "/", "?" and "&" - > > e.g. some of the characters that are "special" in URL? I would like to test > > that we're escaping them properly everywhere. I do not insist on doing that > > as part of this patch though - having separate task under SQOOP-2674 > > umbreall is completely fine :)
The tests for special char are neccessary, and I will do this in a separate task under SQOOP-2674. > On Nov. 10, 2015, 4:07 p.m., Jarek Cecho wrote: > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_3UpgradeTest.java, > > lines 89-93 > > <https://reviews.apache.org/r/40123/diff/1/?file=1121228#file1121228line89> > > > > Shouldn't we convert this to delete job names rather then dropping it > > ocmpletely? The getDeleteJobIds is the same in Derby1_99_3UpgradeTest, Derby1_99_4UpgradeTest, Derby1_99_5UpgradeTest, Derby1_99_6UpgradeTest, so I delete this method and add the same logic in DerbyRepositoryUpgradeTest.testPostUpgrade(), as the following: // Remove all objects for(String name : jobIdToNameMap.values()) { getClient().deleteJob(name); } > On Nov. 10, 2015, 4:07 p.m., Jarek Cecho wrote: > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java, > > lines 162-166 > > <https://reviews.apache.org/r/40123/diff/1/?file=1121232#file1121232line162> > > > > I would drop the idea of "id" from this test completely and refactore > > all the methods to use names. For example, Derby1_99_3UpgradeTest is tested with the repo deby-repository-1.99.3.tar.gz, we can get 6 jobs. But, 4 of 6 have the fixed job name: Job1, Job2, Job3, Export1, the other 2 have the random job name, eg, nonunique_584a8d98-cb72-4354-8100-2b115524f7ae, nonunique_b56865df-95aa-4bbc-8358-6737f1024d93, etc. Do you know why these names will change every time? With the current patch, all methodes use job name, and job id is only for get job name. > On Nov. 10, 2015, 4:07 p.m., Jarek Cecho wrote: > > server/src/main/java/org/apache/sqoop/handler/HandlerUtils.java, lines 30-75 > > <https://reviews.apache.org/r/40123/diff/1/?file=1121222#file1121222line30> > > > > I like how this method is simplified suddenly :) > > > > One point though: The client API is calling > > UrlSafeUtils.urlPathEncode() [1] on the identifier - shouldn't we call > > decode here? > > > > Links: > > 1: > > https://github.com/apache/sqoop/blob/sqoop2/client/src/main/java/org/apache/sqoop/client/request/JobResourceRequest.java#L67 I'll fix it. - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40123/#review105886 ----------------------------------------------------------- On Nov. 10, 2015, 1:43 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40123/ > ----------------------------------------------------------- > > (Updated Nov. 10, 2015, 1:43 a.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Remove the id from public interface for Job > > > Diffs > ----- > > server/src/main/java/org/apache/sqoop/handler/HandlerUtils.java 69bccf7 > server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java > 6face94 > server/src/main/java/org/apache/sqoop/handler/SubmissionRequestHandler.java > 02937bb > test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java > 386b701 > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java > 8e3d7df > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java > a7be9c6 > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_3UpgradeTest.java > 3962449 > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_4UpgradeTest.java > 9ee0379 > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_5UpgradeTest.java > 4183d8b > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_6UpgradeTest.java > 59980c0 > > test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java > 98e1fa1 > > test/src/test/java/org/apache/sqoop/integration/server/SubmissionWithDisabledModelObjectsTest.java > 93cc7f6 > > Diff: https://reviews.apache.org/r/40123/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
