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