> On July 2, 2015, 3:27 p.m., Amareshwari Sriramadasu wrote: > > lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java, > > lines 87-105 > > <https://reviews.apache.org/r/36133/diff/1/?file=998310#file998310line87> > > > > Its same code in all methods. Shall we move this to a private method - > > which takes required params and call from all methods? > > > > Also, seems like we are removing autocommit purposefully. Would > > autocommit give us any advantages? wrt performance? > > Deepak Barr wrote: > Setting autocommit false allows you to rollback on failures. This is > particularly useful in atomic transaction. But in our case, all db updates > are single statement only. So, autocommit=false is not useful for us. > Also,there are no advantages wrt performance as well in our case. > > Originally, the issue was a commit() call in > createFinishedQueriesTable(). Initially I thought that for hsqldb > connections, autocommit is false by default and thats why commit() call does > not throw any Exception. But later I found that all the connections created > from JDBC driver has autocommit=true. Its just that when you call commit() > when autocommit=true, HSQLDB driver does not throw an exception but MySQL > does. In other words, to fix the issue - the minimal change we need to do is > to remove the commit() call in createFinishedQueriesTable(). We can get rid > of the autocommit=false and rollback logics as well. > > Jaideep dhok wrote: > +1 Exception handling code can be refactored into a method. > > Deepak Barr wrote: > Hey Jaideep/Amareshwari, Thank you for your feedback. On the final note, > please advise if I should keep the autocommit=false/rollback logic in the > patch OR should I remove it. Like I had mentioned, autocommit=false/rollback > logic good to have for atomic transactions, but offers no benefit for our > single transaction db updates. > > Amareshwari Sriramadasu wrote: > Can we set autocommit to true explicitly, like it is set to false now? > Will all engines obey that call? > > Deepak Barr wrote: > Amareshwari, > > By default, JDBC connection objects are autocommit=true. So, there is > no need to explicitly set autocommit to true. I cross checked this fact in > oracle documentation as well. > > Amareshwari Sriramadasu wrote: > I'm afraid there can be some engines which may not have that default. Or > people have passed autocommit=false explicitly in jdbc connection string. > > Here are the options i can think: > > 1. Not sure if we want to do this when user explicitly gave autocommit to > false in connection string. > ---- > conn.setAutoCommit(true); > QueryRunner runner = new QueryRunner(); > runner.update(conn, dropQuery); > ---- > > 2. The code done in this patch. Looks fine though lots of code put for > commit, rollback and etc. Do we need rollback even for single line commands? > ---- > conn.setAutoCommit(false); > QueryRunner runner = new QueryRunner(); > runner.update(conn, dropQuery); > conn.commit(); > ---- > 3. If have an option to check autocommit is on or not? Can we use that? > ---- > QueryRunner runner = new QueryRunner(); > runner.update(conn, dropQuery); > if (conn.noAutoCommit()) { > conn.commit(); > } > ---- > > Deepak Barr wrote: > Both 1 & 3 will work for us and can be implemented. I would favor 3rd > option. > > For 2, No we dont need rollback for single statements.
+1 for option 3. Seems we have connection.getAutoCommit() api available. It would simple code. Even in option 3, i'm thinking there would be no rollbacks required. - Amareshwari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36133/#review90232 ----------------------------------------------------------- On July 2, 2015, 2:57 p.m., Deepak Barr wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36133/ > ----------------------------------------------------------- > > (Updated July 2, 2015, 2:57 p.m.) > > > Review request for lens, Jaideep dhok and Pranav Agarwal. > > > Bugs: LENS-639 > https://issues.apache.org/jira/browse/LENS-639 > > > Repository: lens > > > Description > ------- > > Patch for LENS-639. All database updates will be handled by commits & > rollbacks. > > > Diffs > ----- > > lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java > 1904350 > > Diff: https://reviews.apache.org/r/36133/diff/ > > > Testing > ------- > > Yes > > > Thanks, > > Deepak Barr > >
