> 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 :) > > Colin Ma wrote: > The tests for special char are neccessary, and I will do this in a > separate task under SQOOP-2674.
+1, thank you! > 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 > > Colin Ma wrote: > I'll fix it. > > Colin Ma wrote: > Check the code again, RequestContext.getLastURLElement or > RequestContext.getUrlElements will do the decode for the identity. We needn't > do the decode here. Nicely hidden, thanks for checking :) > 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? > > Colin Ma wrote: > 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); > } Got, I haven't realize that. > 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. > > Colin Ma wrote: > 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. The reason for the random name is that in 1.99.3 the name was optional whereas in later versions it became compulsory. So we have a code in upgrade that generates names at random for such objects. I see the need to be a more dynamic here with getting the names, at least for now, so +1. - Jarek ----------------------------------------------------------- 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 > >
