You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Colin Ma <ju...@intel.com> on 2015/11/10 02:43:48 UTC
Review Request 40123: SQOOP-2675: Sqoop2: Remove the id from public
interface for Job
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40123/
-----------------------------------------------------------
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
Re: Review Request 40123: SQOOP-2675: Sqoop2: Remove the id from
public interface for Job
Posted by Jarek Cecho <ja...@apache.org>.
> 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
>
>
Re: Review Request 40123: SQOOP-2675: Sqoop2: Remove the id from
public interface for Job
Posted by Colin Ma <ju...@intel.com>.
> 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
>
>
Re: Review Request 40123: SQOOP-2675: Sqoop2: Remove the id from
public interface for Job
Posted by Colin Ma <ju...@intel.com>.
> 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.
Check the code again, RequestContext.getLastURLElement or RequestContext.getUrlElements will do the decode for the identity. We needn't do the decode here.
- 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
>
>
Re: Review Request 40123: SQOOP-2675: Sqoop2: Remove the id from
public interface for Job
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40123/#review105886
-----------------------------------------------------------
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 :)
server/src/main/java/org/apache/sqoop/handler/HandlerUtils.java (lines 30 - 38)
<https://reviews.apache.org/r/40123/#comment164578>
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
test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/Derby1_99_3UpgradeTest.java
<https://reviews.apache.org/r/40123/#comment164580>
Shouldn't we convert this to delete job names rather then dropping it ocmpletely?
test/src/test/java/org/apache/sqoop/integration/repository/derby/upgrade/DerbyRepositoryUpgradeTest.java (lines 157 - 161)
<https://reviews.apache.org/r/40123/#comment164582>
I would drop the idea of "id" from this test completely and refactore all the methods to use names.
- Jarek Cecho
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
>
>