You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Dian Fu <di...@gmail.com> on 2015/06/08 17:04:59 UTC
Review Request 35216: Signature of method
getConnectorIdFromIdentifier in
HandlerUtils is inconsistent with other methods
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35216/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-2395
https://issues.apache.org/jira/browse/SQOOP-2395
Repository: sqoop-sqoop2
Description
-------
In HandlerUtils.java, there are three methods and the other two methods both take "String identifier, Repository repository" as the signature. Making method getConnectorIdFromIdentifier also taking "String identifier, Repository repository" as the signature will makes the code more consistent.
Diffs
-----
core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java d1537bf
server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java 5128a27
server/src/main/java/org/apache/sqoop/handler/HandlerUtils.java 93ff60b
server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java c96d66d
Diff: https://reviews.apache.org/r/35216/diff/
Testing
-------
Thanks,
Dian Fu
Re: Review Request 35216: Signature of method
getConnectorIdFromIdentifier
in HandlerUtils is inconsistent with other methods
Posted by Dian Fu <di...@gmail.com>.
> On June 9, 2015, 3:06 a.m., richard zhou wrote:
> > It is a good catch.
> > Here is my thought. Since the param repository is GLOBAL, could we avoid passing it to the function, and get it in line the function?
> >
> > public static long getJobIdFromIdentifier(String identifier) {
> > Repository repository = RepositoryManager.getInstance().getRepository();
> > ...
> > }
> >
> > Further, from my perspective, the repository is abused. How about move all repository into HandlerUtils class or sqoop-core component? But it seems a huge refactor. We could split the tasks.
> > 1. move all repository from *RequestHandler into HandlerUtils
> > 2. move all repository from HandlerUtils into ConnectorMangerUtils etc. in sqoop-core component.
Since the param repository is GLOBAL, could we avoid passing it to the function, and get it in line the function?
>> Yes, of course.
1. move all repository from *RequestHandler into HandlerUtils
2. move all repository from HandlerUtils into ConnectorMangerUtils etc. in sqoop-core component.
>> Yes, we can do this in another JIRA.
- Dian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35216/#review87122
-----------------------------------------------------------
On June 8, 2015, 3:04 p.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35216/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 3:04 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2395
> https://issues.apache.org/jira/browse/SQOOP-2395
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> In HandlerUtils.java, there are three methods and the other two methods both take "String identifier, Repository repository" as the signature. Making method getConnectorIdFromIdentifier also taking "String identifier, Repository repository" as the signature will makes the code more consistent.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java d1537bf
> server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java 5128a27
> server/src/main/java/org/apache/sqoop/handler/HandlerUtils.java 93ff60b
> server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java c96d66d
>
> Diff: https://reviews.apache.org/r/35216/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 35216: Signature of method
getConnectorIdFromIdentifier
in HandlerUtils is inconsistent with other methods
Posted by richard zhou <ri...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35216/#review87122
-----------------------------------------------------------
It is a good catch.
Here is my thought. Since the param repository is GLOBAL, could we avoid passing it to the function, and get it in line the function?
public static long getJobIdFromIdentifier(String identifier) {
Repository repository = RepositoryManager.getInstance().getRepository();
...
}
Further, from my perspective, the repository is abused. How about move all repository into HandlerUtils class or sqoop-core component? But it seems a huge refactor. We could split the tasks.
1. move all repository from *RequestHandler into HandlerUtils
2. move all repository from HandlerUtils into ConnectorMangerUtils etc. in sqoop-core component.
- richard zhou
On June 8, 2015, 3:04 p.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35216/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 3:04 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2395
> https://issues.apache.org/jira/browse/SQOOP-2395
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> In HandlerUtils.java, there are three methods and the other two methods both take "String identifier, Repository repository" as the signature. Making method getConnectorIdFromIdentifier also taking "String identifier, Repository repository" as the signature will makes the code more consistent.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java d1537bf
> server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java 5128a27
> server/src/main/java/org/apache/sqoop/handler/HandlerUtils.java 93ff60b
> server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java c96d66d
>
> Diff: https://reviews.apache.org/r/35216/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 35216: Signature of method
getConnectorIdFromIdentifier
in HandlerUtils is inconsistent with other methods
Posted by richard zhou <ri...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35216/#review88937
-----------------------------------------------------------
Ship it!
Ship It!
- richard zhou
On June 11, 2015, 3:05 a.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35216/
> -----------------------------------------------------------
>
> (Updated June 11, 2015, 3:05 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2395
> https://issues.apache.org/jira/browse/SQOOP-2395
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> In HandlerUtils.java, there are three methods and the other two methods both take "String identifier, Repository repository" as the signature. Making method getConnectorIdFromIdentifier also taking "String identifier, Repository repository" as the signature will makes the code more consistent.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java d1537bf
> server/src/main/java/org/apache/sqoop/handler/HandlerUtils.java 718b9e6
> server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java c96d66d
> server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java bf3a42a
> server/src/main/java/org/apache/sqoop/handler/SubmissionRequestHandler.java b4c16f1
>
> Diff: https://reviews.apache.org/r/35216/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 35216: Signature of method
getConnectorIdFromIdentifier
in HandlerUtils is inconsistent with other methods
Posted by Dian Fu <di...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35216/
-----------------------------------------------------------
(Updated June 11, 2015, 3:05 a.m.)
Review request for Sqoop.
Bugs: SQOOP-2395
https://issues.apache.org/jira/browse/SQOOP-2395
Repository: sqoop-sqoop2
Description
-------
In HandlerUtils.java, there are three methods and the other two methods both take "String identifier, Repository repository" as the signature. Making method getConnectorIdFromIdentifier also taking "String identifier, Repository repository" as the signature will makes the code more consistent.
Diffs (updated)
-----
core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java d1537bf
server/src/main/java/org/apache/sqoop/handler/HandlerUtils.java 718b9e6
server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java c96d66d
server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java bf3a42a
server/src/main/java/org/apache/sqoop/handler/SubmissionRequestHandler.java b4c16f1
Diff: https://reviews.apache.org/r/35216/diff/
Testing
-------
Thanks,
Dian Fu