sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "richard zhou" <richard.z...@intel.com>
Subject Re: Review Request 35216: Signature of method getConnectorIdFromIdentifier in HandlerUtils is inconsistent with other methods
Date Tue, 09 Jun 2015 03:06:00 GMT

-----------------------------------------------------------
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
> 
>


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