You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Jarek Cecho <ja...@apache.org> on 2015/11/07 19:43:02 UTC
Review Request 40049: SQOOP-2670 Sqoop2: RESTiliency: Allow getting
links by connector only for all
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40049/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-2670
https://issues.apache.org/jira/browse/SQOOP-2670
Repository: sqoop-sqoop2
Description
-------
I've refactored the method responsible for retrieving links as described on the JIRA.
Diffs
-----
server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java fe0c4d8
server/src/main/java/org/apache/sqoop/server/v1/LinkServlet.java 3fb542f
test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java 856d21f
Diff: https://reviews.apache.org/r/40049/diff/
Testing
-------
I've added bunch of test cases to REST test verifying that we're behaving as expected.
Thanks,
Jarek Cecho
Re: Review Request 40049: SQOOP-2670 Sqoop2: RESTiliency: Allow
getting links by connector only for all
Posted by Jarek Cecho <ja...@apache.org>.
> On Nov. 9, 2015, 6:14 p.m., Abraham Fine wrote:
> > server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java, line 251
> > <https://reviews.apache.org/r/40049/diff/2/?file=1118955#file1118955line251>
> >
> > is there any way we can refactor this so we don't need to check the identifier value twice?
I'm hoping that the second if will get completely dropped once SQOOP-2673 gets in, so I would probably vote to keep the check for now - it will be easier to incorporate that change.
> On Nov. 9, 2015, 6:14 p.m., Abraham Fine wrote:
> > test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java, line 66
> > <https://reviews.apache.org/r/40049/diff/2/?file=1118957#file1118957line66>
> >
> > not super related to your change, but can we refactor this try blok into multiple lines?
Fair point. Hari already committed the patch, so I'll do that in subsequent JIRA SQOOP-2677.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40049/#review105710
-----------------------------------------------------------
On Nov. 7, 2015, 11:25 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40049/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2015, 11:25 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2670
> https://issues.apache.org/jira/browse/SQOOP-2670
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> I've refactored the method responsible for retrieving links as described on the JIRA.
>
>
> Diffs
> -----
>
> server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java fe0c4d8
> server/src/main/java/org/apache/sqoop/server/v1/LinkServlet.java 3fb542f
> test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java 856d21f
>
> Diff: https://reviews.apache.org/r/40049/diff/
>
>
> Testing
> -------
>
> I've added bunch of test cases to REST test verifying that we're behaving as expected.
>
>
> Thanks,
>
> Jarek Cecho
>
>
Re: Review Request 40049: SQOOP-2670 Sqoop2: RESTiliency: Allow
getting links by connector only for all
Posted by Abraham Fine <ab...@brightroll.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40049/#review105710
-----------------------------------------------------------
server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java (line 226)
<https://reviews.apache.org/r/40049/#comment164327>
is there any way we can refactor this so we don't need to check the identifier value twice?
test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java (line 64)
<https://reviews.apache.org/r/40049/#comment164328>
not super related to your change, but can we refactor this try blok into multiple lines?
- Abraham Fine
On Nov. 7, 2015, 11:25 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40049/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2015, 11:25 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2670
> https://issues.apache.org/jira/browse/SQOOP-2670
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> I've refactored the method responsible for retrieving links as described on the JIRA.
>
>
> Diffs
> -----
>
> server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java fe0c4d8
> server/src/main/java/org/apache/sqoop/server/v1/LinkServlet.java 3fb542f
> test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java 856d21f
>
> Diff: https://reviews.apache.org/r/40049/diff/
>
>
> Testing
> -------
>
> I've added bunch of test cases to REST test verifying that we're behaving as expected.
>
>
> Thanks,
>
> Jarek Cecho
>
>
Re: Review Request 40049: SQOOP-2670 Sqoop2: RESTiliency: Allow
getting links by connector only for all
Posted by Jarek Cecho <ja...@apache.org>.
> On Nov. 8, 2015, 7:52 a.m., Abraham Elmahrek wrote:
> > Good stuff... just two thoughts...
Thanks for the review Abe!
I would prefer not to mess with HTTP return codes as part of this JIRA. We're using one simple HTTP code for all exceptions that are being sent back from server and we're using HTTP code 500 for that. We can look into sending various other codes, but that seems orthogonal to this JIRA. Would you agree?
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40049/#review105605
-----------------------------------------------------------
On Nov. 7, 2015, 11:25 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40049/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2015, 11:25 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2670
> https://issues.apache.org/jira/browse/SQOOP-2670
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> I've refactored the method responsible for retrieving links as described on the JIRA.
>
>
> Diffs
> -----
>
> server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java fe0c4d8
> server/src/main/java/org/apache/sqoop/server/v1/LinkServlet.java 3fb542f
> test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java 856d21f
>
> Diff: https://reviews.apache.org/r/40049/diff/
>
>
> Testing
> -------
>
> I've added bunch of test cases to REST test verifying that we're behaving as expected.
>
>
> Thanks,
>
> Jarek Cecho
>
>
Re: Review Request 40049: SQOOP-2670 Sqoop2: RESTiliency: Allow
getting links by connector only for all
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40049/#review105605
-----------------------------------------------------------
Ship it!
Good stuff... just two thoughts...
server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java (line 212)
<https://reviews.apache.org/r/40049/#comment164231>
Follow up Jira to make this a 400?
test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java (line 213)
<https://reviews.apache.org/r/40049/#comment164229>
404?
test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java (line 220)
<https://reviews.apache.org/r/40049/#comment164230>
400?
- Abraham Elmahrek
On Nov. 7, 2015, 11:25 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40049/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2015, 11:25 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2670
> https://issues.apache.org/jira/browse/SQOOP-2670
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> I've refactored the method responsible for retrieving links as described on the JIRA.
>
>
> Diffs
> -----
>
> server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java fe0c4d8
> server/src/main/java/org/apache/sqoop/server/v1/LinkServlet.java 3fb542f
> test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java 856d21f
>
> Diff: https://reviews.apache.org/r/40049/diff/
>
>
> Testing
> -------
>
> I've added bunch of test cases to REST test verifying that we're behaving as expected.
>
>
> Thanks,
>
> Jarek Cecho
>
>
Re: Review Request 40049: SQOOP-2670 Sqoop2: RESTiliency: Allow
getting links by connector only for all
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40049/
-----------------------------------------------------------
(Updated Nov. 7, 2015, 11:25 p.m.)
Review request for Sqoop.
Changes
-------
Fixing precommit hook failures.
Bugs: SQOOP-2670
https://issues.apache.org/jira/browse/SQOOP-2670
Repository: sqoop-sqoop2
Description
-------
I've refactored the method responsible for retrieving links as described on the JIRA.
Diffs (updated)
-----
server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java fe0c4d8
server/src/main/java/org/apache/sqoop/server/v1/LinkServlet.java 3fb542f
test/src/test/java/org/apache/sqoop/integration/server/InvalidRESTCallsTest.java 856d21f
Diff: https://reviews.apache.org/r/40049/diff/
Testing
-------
I've added bunch of test cases to REST test verifying that we're behaving as expected.
Thanks,
Jarek Cecho