> On Oct. 27, 2014, 10:30 p.m., Veena Basavaraj wrote:
> > core/src/main/java/org/apache/sqoop/repository/Repository.java, line 472
> > <https://reviews.apache.org/r/26609/diff/2/?file=734640#file734640line472>
> >
> > cant we take the Direction as an argument, return boolean and extract
> > these lines into a methods? so that froma nd to job validation can be
> > shared. there are places which has done this.
> >
> > If you think it is overkill, I can do it some point later. not a must
I've actually tried that before, but I was not happy with the extracted method:
private final boolean verifyAndUpdateJob(MJob job, RepositoryTransaction tx,
SqoopConnector connector, Direction direction) {
ConfigValidationResult validationResult = ConfigUtils.validateConfigs(
job.getJobConfig(direction).getConfigs(),
connector.getJobConfigurationClass(direction)
);
if(validationResult.getStatus().canProceed()) {
updateJob(job, tx);
return true;
} else {
logInvalidModelObject("job", job, validationResult);
return false;
}
I feel that the method is taking too much arguments for very little of
effective code, so I felt that it's more obvious to have that code "inline".
Happy to put the method back if you feel that it's better this way.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26609/#review58698
-----------------------------------------------------------
On Oct. 27, 2014, 9:57 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26609/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2014, 9:57 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1576
> https://issues.apache.org/jira/browse/SQOOP-1576
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> I've migrated Repository validation structure to the new infra. I've noticed
> some code reuse between Repository and Server Handlers, so I've refactored
> those to ConfigUtils class.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/model/ConfigUtils.java 372e7d3
> common/src/main/java/org/apache/sqoop/model/ModelError.java 35a8943
> common/src/test/java/org/apache/sqoop/model/TestConfigUtils.java 2a7281a
> core/src/main/java/org/apache/sqoop/repository/Repository.java bd2a3be
> core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java
> e11fd10
> server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java
> 61b1610
> server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java
> b715ad3
>
> Diff: https://reviews.apache.org/r/26609/diff/
>
>
> Testing
> -------
>
> Unit tests seems to be passing.
>
>
> Thanks,
>
> Jarek Cecho
>
>
