sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request 40522: SQOOP-2690: Sqoop2: Use connector name in MLink.
Date Wed, 25 Nov 2015 16:18:58 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40522/#review108012
-----------------------------------------------------------


Thanks for pushing this one forward:


client/src/main/java/org/apache/sqoop/client/SqoopClient.java (lines 400 - 401)
<https://reviews.apache.org/r/40522/#comment167384>

    To follow up the discussion we had on previous patch - I don't think that we should return
reference to the internal structures here as you found out that it means that we can't call
more then one createJob(). I think that we should return copy of the structures instead, e.g.:
    
    connectorForFromLink.getFromConfig().clone(false)
    
    What do you think?



repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java
(line 1520)
<https://reviews.apache.org/r/40522/#comment167387>

    Nit: Unused variable?



repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java
(line 1568)
<https://reviews.apache.org/r/40522/#comment167388>

    Nit: Unused variable?



test/src/main/java/org/apache/sqoop/test/testcases/JettyTestCase.java (lines 106 - 109)
<https://reviews.apache.org/r/40522/#comment167381>

    I have a concern that this is hiding the real problem - that we're returning mutable objects
that are referencing cache in SqoopClient. Let's get it fixed there rather then doing this.


Jarcec

- Jarek Cecho


On Nov. 23, 2015, 4:40 a.m., Colin Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40522/
> -----------------------------------------------------------
> 
> (Updated Nov. 23, 2015, 4:40 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> Currently connector id is used in MLink as a foreigner key, the connector id will be
removed from public API, this should be updated as connector name.
> 
> 
> Diffs
> -----
> 
>   client/src/main/java/org/apache/sqoop/client/SqoopClient.java 3d3425d 
>   client/src/main/java/org/apache/sqoop/client/request/ConnectorResourceRequest.java
f23c6e4 
>   client/src/main/java/org/apache/sqoop/client/request/SqoopResourceRequests.java 0987703

>   client/src/test/java/org/apache/sqoop/client/TestSqoopClient.java c962358 
>   common/src/main/java/org/apache/sqoop/json/ConnectorBean.java 89ae20c 
>   common/src/main/java/org/apache/sqoop/json/ConnectorsBean.java 9762ffa 
>   common/src/main/java/org/apache/sqoop/json/LinkBean.java c9a8ab3 
>   common/src/main/java/org/apache/sqoop/model/MLink.java 51a4008 
>   common/src/test/java/org/apache/sqoop/json/TestConnectorBean.java a978d62 
>   common/src/test/java/org/apache/sqoop/json/TestConnectorsBean.java 15377c9 
>   common/src/test/java/org/apache/sqoop/json/util/BeanTestUtil.java 51ab0e5 
>   common/src/test/java/org/apache/sqoop/model/TestMAccountableEntity.java 5bf2465 
>   common/src/test/java/org/apache/sqoop/model/TestMLink.java b5df18a 
>   core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java db626a1 
>   core/src/main/java/org/apache/sqoop/driver/JobManager.java 90ee541 
>   core/src/test/java/org/apache/sqoop/driver/TestJobManager.java f137eef 
>   core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 00b0511 
>   repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java
d1ee320 
>   repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInputTypes.java
193ee5f 
>   repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java
92d1fae 
>   repository/repository-mysql/src/test/java/org/apache/sqoop/integration/repository/mysql/MySqlTestCase.java
232ef4c 
>   repository/repository-postgresql/src/test/java/org/apache/sqoop/integration/repository/postgresql/PostgresqlTestCase.java
b8b0f52 
>   server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java 52abe72

>   server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java 9e75258 
>   shell/src/main/java/org/apache/sqoop/shell/CloneJobFunction.java 15cfad7 
>   shell/src/main/java/org/apache/sqoop/shell/CloneLinkFunction.java a679be8 
>   shell/src/main/java/org/apache/sqoop/shell/CreateJobFunction.java 79caa0d 
>   shell/src/main/java/org/apache/sqoop/shell/CreateLinkFunction.java 289c3c3 
>   shell/src/main/java/org/apache/sqoop/shell/ShowConnectorFunction.java 6efb51c 
>   shell/src/main/java/org/apache/sqoop/shell/ShowJobFunction.java 21873cc 
>   shell/src/main/java/org/apache/sqoop/shell/ShowLinkFunction.java 04dd228 
>   shell/src/main/java/org/apache/sqoop/shell/UpdateJobFunction.java 49cfd0b 
>   shell/src/main/java/org/apache/sqoop/shell/UpdateLinkFunction.java 1bb7cd5 
>   shell/src/test/java/org/apache/sqoop/shell/TestCloneCommand.java eed27d7 
>   shell/src/test/java/org/apache/sqoop/shell/TestCreateCommand.java 48f646b 
>   shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java 4272386 
>   shell/src/test/java/org/apache/sqoop/shell/TestUpdateCommand.java 9b3e87a 
>   test/src/main/java/org/apache/sqoop/test/testcases/JettyTestCase.java bd4ba6a 
>   test/src/test/java/org/apache/sqoop/integration/connector/hdfs/S3Test.java 7fec310

>   test/src/test/java/org/apache/sqoop/integration/server/ShowJobInOrderTest.java cbf1e90

>   tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java 75d2182 
> 
> Diff: https://reviews.apache.org/r/40522/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Colin Ma
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message