> 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.
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();
}
----
- 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
>
>
