> On 2012-05-10 17:21:37, Alejandro Abdelnur wrote: > > Looks good, I've made some comments inline. > > > > In addition: > > > > * I don't see the different oozie-site.xml for testing for different DBs > > being updated, is it not required? > > > > After your feedback I'll take the patch for a spin doing upgrades and > > creation tests. BTW, have you done that? > > > > Thanks, nice work. > >
I don't see the different oozie-site.xml for testing for different DBs being updated, is it not required? -- You mean why there is not a "oozie.db.schema.name" property in oozie-site.xml? It is not requried, and if any wants to specify the schema, he must configure it. if not, the database will choose the default schema itself. After your feedback I'll take the patch for a spin doing upgrades and creation tests. BTW, have you done that? -- Yes, i am sure you would not feel bothering when you test with this patch. > On 2012-05-10 17:21:37, Alejandro Abdelnur wrote: > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/resources/META-INF/persistence.xml, > > line 34 > > <https://reviews.apache.org/r/4755/diff/2/?file=107962#file107962line34> > > > > please fix the indentation of these 3 classes ok > On 2012-05-10 17:21:37, Alejandro Abdelnur wrote: > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/resources/META-INF/persistence.xml, > > line 91 > > <https://reviews.apache.org/r/4755/diff/2/?file=107962#file107962line91> > > > > indentation fix ok > On 2012-05-10 17:21:37, Alejandro Abdelnur wrote: > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/resources/META-INF/persistence.xml, > > line 148 > > <https://reviews.apache.org/r/4755/diff/2/?file=107962#file107962line148> > > > > indentation fix ok > On 2012-05-10 17:21:37, Alejandro Abdelnur wrote: > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/resources/META-INF/persistence.xml, > > line 205 > > <https://reviews.apache.org/r/4755/diff/2/?file=107962#file107962line205> > > > > indentation fix ok > On 2012-05-10 17:21:37, Alejandro Abdelnur wrote: > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/resources/oozie-default.xml, > > line 1002 > > <https://reviews.apache.org/r/4755/diff/2/?file=107963#file107963line1002> > > > > you need to leave a space as value, else the value will be NULL. > > Looking at the code it really does not matter. So, this is just a minor NIT take it, :D > On 2012-05-10 17:21:37, Alejandro Abdelnur wrote: > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/docs/src/site/twiki/AG_Install.twiki, > > line 119 > > <https://reviews.apache.org/r/4755/diff/2/?file=107964#file107964line119> > > > > we should add here the oozie.db.schema.name property with the default > > value, right? There is no need. Databases differ in their default shema, that of postgres is public and of derby is APP. Database will choose the default schema itselt, if it is not specified. > On 2012-05-10 17:21:37, Alejandro Abdelnur wrote: > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/tools/src/main/java/org/apache/oozie/tools/OozieDBCLI.java, > > line 169 > > <https://reviews.apache.org/r/4755/diff/2/?file=107965#file107965line169> > > > > we should trim the value obtained from the conf.get() here ok > On 2012-05-10 17:21:37, Alejandro Abdelnur wrote: > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/tools/src/main/java/org/apache/oozie/tools/OozieDBCLI.java, > > line 236 > > <https://reviews.apache.org/r/4755/diff/2/?file=107965#file107965line236> > > > > can we please leave the names of table and columns with the case > > defined in the bean as they where before? else we could run into issues > > when a DB is case sensitive for table/column names. i don't want to add the schema to each sql, that's why i take the jpa. And also for this reason, we should use the JPA Query so that jpa will add the schema auto in each query. i not sure i have understood you question, i wish the reply is that you wonder. > On 2012-05-10 17:21:37, Alejandro Abdelnur wrote: > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/tools/src/main/java/org/apache/oozie/tools/OozieDBCLI.java, > > line 320 > > <https://reviews.apache.org/r/4755/diff/2/?file=107965#file107965line320> > > > > why is this being remove? OpenJPA doe not do this as part of an > > upgrade, that is why this is here I'm sorry, i made a serious mistake here. This part is recovered with little change in the new patch. > On 2012-05-10 17:21:37, Alejandro Abdelnur wrote: > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/tools/src/main/java/org/apache/oozie/tools/OozieDBCLI.java, > > line 645 > > <https://reviews.apache.org/r/4755/diff/2/?file=107965#file107965line645> > > > > trim value of conf.get() ok - Han ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4755/#review7779 ----------------------------------------------------------- On 2012-05-09 02:36:58, Han Xiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4755/ > ----------------------------------------------------------- > > (Updated 2012-05-09 02:36:58) > > > Review request for oozie. > > > Summary > ------- > > Oozie's JPA service doesn't support configure different schemas for oozie's > db. > Our company wants to use one db for multi oozie clusters, and then we want > each oozie cluster can use different schema of the db. Therefore JPAService > in oozie should support to configure the schema of the db, not just the > database. > The oozie.db.schema.name is use to configure oozie's db name, however, it is > a little confusing for it is not used to configure the actually schema of db. > > > This addresses bug OOZIE-814. > https://issues.apache.org/jira/browse/OOZIE-814 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/tools/src/main/java/org/apache/oozie/tools/OozieDBCLI.java > 1335881 > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/conf/oozie-site.xml > 1335881 > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/SystemInfoBean.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/service/JPAService.java > 1335881 > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/resources/META-INF/persistence.xml > 1335881 > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/resources/oozie-default.xml > 1335881 > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/docs/src/site/twiki/AG_Install.twiki > 1335881 > > http://svn.apache.org/repos/asf/incubator/oozie/trunk/utils/dbutils/updatescripts/readme.txt > 1335881 > > Diff: https://reviews.apache.org/r/4755/diff > > > Testing > ------- > > * Deployed Oozie, added oozie.db.schema.name configs values, succeed to run > the examples on Derby and Postgres databases. > > > Thanks, > > Han > >
