You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Filippo Causa <fi...@unicredit.eu> on 2015/09/02 12:34:44 UTC
Re: Review Request 36729: Patch for jira 1096
> On Aug. 8, 2015, 6:42 p.m., Jarek Cecho wrote:
> > Hi Thomas ,
> > thank you for the patch! I have two high level notes and several nits:
> >
> > 1) I believe that we should document the new arguments in our user guide [1]. I would not hesitate to explain the use case with the WITH UR on DB2 connector that you hit.
> > 2) I'm missing tests for the new functionality, would you mind adding some?
> >
> > Links:
> > 1: https://github.com/apache/sqoop/tree/trunk/src/docs/user
>
> Filippo Causa wrote:
> thanks for your tips. Sorry but my name is not Thomas but Filippo. In the next days i will work to improve the patch to your specification.
>
> Filippo
>
> Jarek Cecho wrote:
> So embarassing, my huge apologies for messing your name Filippo!
>
> Venkat Ranganathan wrote:
> Just to point out, WITH UR is WITH UNCOMMITTED READ option and the same effect can be had with --relaxed-isolation option. Also this was specific to DB2, supporting this syntax might be good to do in the DB2Manager?
>
> Jarek Cecho wrote:
> I was thinking about proposing change only to DB2Manager for the "WITH UR", but then I've realized that adding custom prefixes and suffixes might be useful to specify various flags in other databases as well. I do not have a concerete example, but MySQL hints that are specified in /* */ can be entered this way. Hence I feel that albeit this is super advanced feature, it could be generally useful. What do you think Venkat?
>
> Filippo Causa wrote:
> I think --relaxed-isoltaion is good idea but working only in the mapper step, so it would be made available also in the process of calculation of the split and "select max()" . We could add both features ( isolation , suffix and alias arguments) to provide global capability. What do you think?
>
> Venkat Ranganathan wrote:
> Hi Jarcec, not sure if MySQL Hints require that they have to be at the end only (unlike the DB2 syntax). This feature may generally be useful, but not sure for this? WITH UR option is specific to DB2 and may be easily solved in DB2 connection manager or better yet we tell people not to use WITH UR option but used --relaxed-isolation for queries? --relaxed-isolation also works with table imports (you don't have to craft a query for that).
> Filippo Causa, good point about --relaxed-isolation not working with max queries. We should definitely fix those but generally DBs would handle this but I agree, better to fix Sqoop code to make sure this feature also works for queries executed in the client
>
> Filippo Causa wrote:
> If we all agree isolation-level approach, i am fixing the code to add relaxed-isolation in max queries and split queries. So we can close the jira and suggest the use of relaxed-isolation parameters to work with db2 instead of WITH UR. Then, we can open a new jira to add prefix,suffix and alias arguments as new features. Prefix arguments can be useful for statement like "with v as (select * from t1) select a,b from t2 inner join v".
>
> Jarek Cecho wrote:
> Seems reasonable to me Filippo.
>
> Filippo Causa wrote:
> I have uploaded a new patch. Sorry if it's not right way to update the review request.
>
> Venkat Ranganathan wrote:
> Agreed - that is the reasonable approach. Thanks
>
> Jarek Cecho wrote:
> Thanks Filippo for the updated patch. I believe that the agreement is to make this change specific to DB2 connector (e.g. add extra argument --with-ur or something) and take the general approach to separate JIRA, correct?
Jarek sorry ,I had understood that other jira were to add the parameters prefix, suffix. I thought that in this case it was not necessary to make a change only for the db2 as with-ur statement changes the isolation level that is common to all rdbms , so I extended setting the isolation level to the other connections always based on the parameter relaxed-isolation . So we could solve the problem with minimal impact and add to the documentation that " with ur " is just the same as relaxed-isolation. What do you think?
> On Aug. 8, 2015, 6:42 p.m., Jarek Cecho wrote:
> > src/java/com/cloudera/sqoop/mapreduce/db/DBConfiguration.java, lines 82-84
> > <https://reviews.apache.org/r/36729/diff/1/?file=1019696#file1019696line82>
> >
> > We should not longer add new constants to the com.cloudera.sqoop classes - they are kept for backward compatibility only.
it will be moved to new jira
> On Aug. 8, 2015, 6:42 p.m., Jarek Cecho wrote:
> > src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java, lines 66-71
> > <https://reviews.apache.org/r/36729/diff/1/?file=1019697#file1019697line66>
> >
> > Same note here about com.cloudera.sqoop classes.
it will be moved to new jira
> On Aug. 8, 2015, 6:42 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/SqoopOptions.java, line 145
> > <https://reviews.apache.org/r/36729/diff/1/?file=1019698#file1019698line145>
> >
> > Allow user to specify custom alias seems as a good idea, but I would suggest to do it as part of a different JIRA.
it will be moved to new jira
> On Aug. 8, 2015, 6:42 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/SqoopOptions.java, lines 1206-1210
> > <https://reviews.apache.org/r/36729/diff/1/?file=1019698#file1019698line1206>
> >
> > Let's simplify it with ? : operator as the patch have in getUseAlias() method?
it will be moved to new jira
> On Aug. 8, 2015, 6:42 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java, lines 305-306
> > <https://reviews.apache.org/r/36729/diff/1/?file=1019702#file1019702line305>
> >
> > Seems as change not relevant to the patch?
it will be moved to new jira
> On Aug. 8, 2015, 6:42 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java, lines 385-397
> > <https://reviews.apache.org/r/36729/diff/1/?file=1019704#file1019704line385>
> >
> > Nit: The formatting seems to be off here.
it will be moved to new jira
> On Aug. 8, 2015, 6:42 p.m., Jarek Cecho wrote:
> > src/java/org/apache/sqoop/mapreduce/db/DataDrivenDBRecordReader.java, line 139
> > <https://reviews.apache.org/r/36729/diff/1/?file=1019708#file1019708line139>
> >
> > Nit: Trailing whitespaces
it will be moved to new jira
- Filippo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36729/#review94640
-----------------------------------------------------------
On Aug. 17, 2015, 9:48 a.m., Filippo Causa wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36729/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2015, 9:48 a.m.)
>
>
> Review request for Sqoop and Jarek Cecho.
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> propagated relaxed-isolation parameter setting to connection used for calculating max incremental value and split range. Use --relaxed-isoltaion inbstead of "WITH UR" clause.
>
>
> Diffs
> -----
>
> src/java/com/cloudera/sqoop/mapreduce/db/DBConfiguration.java 89f2b4f
> src/java/com/cloudera/sqoop/tool/BaseSqoopTool.java a5f72f7
> src/java/org/apache/sqoop/SqoopOptions.java 9405605
> src/java/org/apache/sqoop/hive/TableDefWriter.java c9962e9
> src/java/org/apache/sqoop/manager/MySQLManager.java e1d5a36
> src/java/org/apache/sqoop/manager/OracleManager.java 69b613f
> src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java 260bc29
> src/java/org/apache/sqoop/mapreduce/JdbcExportJob.java 93d438a
> src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java a9b7e42
> src/java/org/apache/sqoop/mapreduce/db/DBInputFormat.java 3a8e5d0
> src/java/org/apache/sqoop/mapreduce/db/DBRecordReader.java a78eb06
> src/java/org/apache/sqoop/mapreduce/db/DataDrivenDBInputFormat.java db96e41
> src/java/org/apache/sqoop/mapreduce/db/DataDrivenDBRecordReader.java b734e05
> src/java/org/apache/sqoop/mapreduce/db/OracleDBRecordReader.java 9058a55
> src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java 4070c24
> src/java/org/apache/sqoop/mapreduce/sqlserver/SqlServerRecordReader.java bc101c5
> src/java/org/apache/sqoop/orm/ClassWriter.java 1c6f7f4
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 4e2e66d
> src/java/org/apache/sqoop/tool/ImportTool.java 39af42c
>
> Diff: https://reviews.apache.org/r/36729/diff/
>
>
> Testing
> -------
>
> Test done with jdbc type 4
>
>
> File Attachments
> ----------------
>
> Transaction-Isolation Patch
> https://reviews.apache.org/media/uploaded/files/2015/08/13/9ebd4663-592d-448f-a888-94ee00ca84de__1096.patch
>
>
> Thanks,
>
> Filippo Causa
>
>