sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Filippo Causa" <filippo.ca...@unicredit.eu>
Subject Re: Review Request 36729: Patch for jira 1096
Date Wed, 02 Sep 2015 10:34:44 GMT


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


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