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/02 10:35:53 UTC
Review Request 34930: SQOOP-2386: Sqoop2: refactor code in
HandlerUtils
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34930/
-----------------------------------------------------------
Review request for Sqoop.
Repository: sqoop-sqoop2
Description
-------
In HandlerUtils, there is code like this:
if (repository.findJob(identifier) != null) {
jobId = repository.findJob(identifier).getPersistenceId();
...
}
This will execute repository.findJob twice, this is unnecessary.
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/34930/diff/
Testing
-------
Thanks,
Dian Fu
Re: Review Request 34930: SQOOP-2386: Sqoop2: refactor code in
HandlerUtils
Posted by Dian Fu <di...@gmail.com>.
> On June 3, 2015, 7:43 a.m., Abraham Elmahrek wrote:
> > server/src/main/java/org/apache/sqoop/handler/HandlerUtils.java, line 71
> > <https://reviews.apache.org/r/34930/diff/1/?file=976396#file976396line71>
> >
> > What's the difference between using `Repository` versus `ConnectorManager`?
>
> Dian Fu wrote:
> ConnectorManager will call Repository to complete its work, so the function is identical. 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.
>
> Abraham Elmahrek wrote:
> If it's just clean up, let's make this change in a separate Jira?
OK. Have updated the patch and removed changes related to this. Will address this in SQOOP-2395.
- Dian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34930/#review86375
-----------------------------------------------------------
On June 2, 2015, 8:35 a.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34930/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 8:35 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> In HandlerUtils, there is code like this:
> if (repository.findJob(identifier) != null) {
> jobId = repository.findJob(identifier).getPersistenceId();
> ...
> }
> This will execute repository.findJob twice, this is unnecessary.
>
>
> 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/34930/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 34930: SQOOP-2386: Sqoop2: refactor code in
HandlerUtils
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On June 3, 2015, 7:43 a.m., Abraham Elmahrek wrote:
> > server/src/main/java/org/apache/sqoop/handler/HandlerUtils.java, line 71
> > <https://reviews.apache.org/r/34930/diff/1/?file=976396#file976396line71>
> >
> > What's the difference between using `Repository` versus `ConnectorManager`?
>
> Dian Fu wrote:
> ConnectorManager will call Repository to complete its work, so the function is identical. 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.
If it's just clean up, let's make this change in a separate Jira?
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34930/#review86375
-----------------------------------------------------------
On June 2, 2015, 8:35 a.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34930/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 8:35 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> In HandlerUtils, there is code like this:
> if (repository.findJob(identifier) != null) {
> jobId = repository.findJob(identifier).getPersistenceId();
> ...
> }
> This will execute repository.findJob twice, this is unnecessary.
>
>
> 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/34930/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 34930: SQOOP-2386: Sqoop2: refactor code in
HandlerUtils
Posted by Dian Fu <di...@gmail.com>.
> On June 3, 2015, 7:43 a.m., Abraham Elmahrek wrote:
> > server/src/main/java/org/apache/sqoop/handler/HandlerUtils.java, line 71
> > <https://reviews.apache.org/r/34930/diff/1/?file=976396#file976396line71>
> >
> > What's the difference between using `Repository` versus `ConnectorManager`?
ConnectorManager will call Repository to complete its work, so the function is identical. 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.
- Dian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34930/#review86375
-----------------------------------------------------------
On June 2, 2015, 8:35 a.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34930/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 8:35 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> In HandlerUtils, there is code like this:
> if (repository.findJob(identifier) != null) {
> jobId = repository.findJob(identifier).getPersistenceId();
> ...
> }
> This will execute repository.findJob twice, this is unnecessary.
>
>
> 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/34930/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 34930: SQOOP-2386: Sqoop2: refactor code in
HandlerUtils
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34930/#review86375
-----------------------------------------------------------
server/src/main/java/org/apache/sqoop/handler/HandlerUtils.java
<https://reviews.apache.org/r/34930/#comment138353>
What's the difference between using `Repository` versus `ConnectorManager`?
- Abraham Elmahrek
On June 2, 2015, 8:35 a.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34930/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 8:35 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> In HandlerUtils, there is code like this:
> if (repository.findJob(identifier) != null) {
> jobId = repository.findJob(identifier).getPersistenceId();
> ...
> }
> This will execute repository.findJob twice, this is unnecessary.
>
>
> 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/34930/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 34930: SQOOP-2386: Sqoop2: refactor code in
HandlerUtils
Posted by richard zhou <ri...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34930/#review87123
-----------------------------------------------------------
Ship it!
LGTM
- richard zhou
On June 8, 2015, 2:49 p.m., Dian Fu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34930/
> -----------------------------------------------------------
>
> (Updated June 8, 2015, 2:49 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: sqoop-2386
> https://issues.apache.org/jira/browse/sqoop-2386
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> In HandlerUtils, there is code like this:
> if (repository.findJob(identifier) != null) {
> jobId = repository.findJob(identifier).getPersistenceId();
> ...
> }
> This will execute repository.findJob twice, this is unnecessary.
>
>
> Diffs
> -----
>
> server/src/main/java/org/apache/sqoop/handler/HandlerUtils.java 93ff60b
>
> Diff: https://reviews.apache.org/r/34930/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dian Fu
>
>
Re: Review Request 34930: SQOOP-2386: Sqoop2: refactor code in
HandlerUtils
Posted by Dian Fu <di...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34930/
-----------------------------------------------------------
(Updated June 8, 2015, 2:49 p.m.)
Review request for Sqoop.
Bugs: sqoop-2386
https://issues.apache.org/jira/browse/sqoop-2386
Repository: sqoop-sqoop2
Description
-------
In HandlerUtils, there is code like this:
if (repository.findJob(identifier) != null) {
jobId = repository.findJob(identifier).getPersistenceId();
...
}
This will execute repository.findJob twice, this is unnecessary.
Diffs (updated)
-----
server/src/main/java/org/apache/sqoop/handler/HandlerUtils.java 93ff60b
Diff: https://reviews.apache.org/r/34930/diff/
Testing
-------
Thanks,
Dian Fu
Re: Review Request 34930: SQOOP-2386: Sqoop2: refactor code in
HandlerUtils
Posted by Dian Fu <di...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34930/
-----------------------------------------------------------
(Updated June 8, 2015, 2:48 p.m.)
Review request for Sqoop.
Bugs: sqoop-2386
https://issues.apache.org/jira/browse/sqoop-2386
Repository: sqoop-sqoop2
Description
-------
In HandlerUtils, there is code like this:
if (repository.findJob(identifier) != null) {
jobId = repository.findJob(identifier).getPersistenceId();
...
}
This will execute repository.findJob twice, this is unnecessary.
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/34930/diff/
Testing
-------
Thanks,
Dian Fu