You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lens.apache.org by Deepak Barr <de...@gmail.com> on 2015/07/02 16:57:23 UTC
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
-----------------------------------------------------------
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
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Amareshwari Sriramadasu <am...@apache.org>.
> 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.
Can we set autocommit to true explicitly, like it is set to false now? Will all engines obey that call?
- 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
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Amareshwari Sriramadasu <am...@apache.org>.
> 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
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Amareshwari Sriramadasu <am...@apache.org>.
> 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
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Jaideep dhok <ja...@inmobi.com>.
> 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.
+1 Exception handling code can be refactored into a method.
- Jaideep
-----------------------------------------------------------
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
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Deepak Barr <de...@gmail.com>.
> 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?
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.
- Deepak
-----------------------------------------------------------
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
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Deepak Barr <de...@gmail.com>.
> 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();
> }
> ----
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.
- Deepak
-----------------------------------------------------------
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
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Deepak Barr <de...@gmail.com>.
> 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.
>
> Amareshwari Sriramadasu wrote:
> +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.
Yes, no rollbacks required with 3rd option. I will create a new patch with option 3 then.
- Deepak
-----------------------------------------------------------
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
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Deepak Barr <de...@gmail.com>.
> 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?
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.
- Deepak
-----------------------------------------------------------
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
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Deepak Barr <de...@gmail.com>.
> 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.
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.
- Deepak
-----------------------------------------------------------
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
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Amareshwari Sriramadasu <am...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36133/#review90232
-----------------------------------------------------------
lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java (lines 77 - 95)
<https://reviews.apache.org/r/36133/#comment143253>
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?
- Amareshwari Sriramadasu
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
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Deepak Barr <de...@gmail.com>.
> On July 24, 2015, 8 a.m., Amareshwari Sriramadasu wrote:
> > Deepak, did you get a chance to update this patch?
Not yet. I had made the new patch, now getting internally reviewed in flipkart. Give me one more day
- Deepak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36133/#review92879
-----------------------------------------------------------
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
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Amareshwari Sriramadasu <am...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36133/#review92879
-----------------------------------------------------------
Deepak, did you get a chance to update this patch?
- Amareshwari Sriramadasu
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
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Jaideep dhok <ja...@inmobi.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36133/#review90438
-----------------------------------------------------------
Ship it!
Is it possible to add a test case that fails without the patch?
Otherwise, patch looks good.
- Jaideep dhok
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
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Deepak Barr <de...@gmail.com>.
> On July 30, 2015, 12:11 p.m., Amareshwari Sriramadasu wrote:
> > incubator-lens/lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java, line 71
> > <https://reviews.apache.org/r/36133/diff/2/?file=1023412#file1023412line71>
> >
> > Is this not breaking local setup and tests?
No, it won't break. The table would get created even in local setup. I have just moved the statements in createTable method to createFinishedQueriesTable(). There were just two statements in createTable() and the method was called by createFinishedQueriesTable() method only. The createTable() was not offering reusability.
- Deepak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36133/#review93576
-----------------------------------------------------------
On July 28, 2015, 6:58 a.m., Deepak Barr wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36133/
> -----------------------------------------------------------
>
> (Updated July 28, 2015, 6:58 a.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
> -----
>
> incubator-lens/lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java 1904350
> incubator-lens/lens-server/src/main/java/org/apache/lens/server/util/UtilityMethods.java 9c386a6
>
> Diff: https://reviews.apache.org/r/36133/diff/
>
>
> Testing
> -------
>
> Yes
>
>
> Thanks,
>
> Deepak Barr
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Amareshwari Sriramadasu <am...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36133/#review93576
-----------------------------------------------------------
incubator-lens/lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java
<https://reviews.apache.org/r/36133/#comment147958>
Is this not breaking local setup and tests?
- Amareshwari Sriramadasu
On July 28, 2015, 6:58 a.m., Deepak Barr wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36133/
> -----------------------------------------------------------
>
> (Updated July 28, 2015, 6:58 a.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
> -----
>
> incubator-lens/lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java 1904350
> incubator-lens/lens-server/src/main/java/org/apache/lens/server/util/UtilityMethods.java 9c386a6
>
> Diff: https://reviews.apache.org/r/36133/diff/
>
>
> Testing
> -------
>
> Yes
>
>
> Thanks,
>
> Deepak Barr
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Amareshwari Sriramadasu <am...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36133/#review93866
-----------------------------------------------------------
Ship it!
Please update "Reactor Summary" of "mvn clean install" .
- Amareshwari Sriramadasu
On July 28, 2015, 6:58 a.m., Deepak Barr wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36133/
> -----------------------------------------------------------
>
> (Updated July 28, 2015, 6:58 a.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
> -----
>
> incubator-lens/lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java 1904350
> incubator-lens/lens-server/src/main/java/org/apache/lens/server/util/UtilityMethods.java 9c386a6
>
> Diff: https://reviews.apache.org/r/36133/diff/
>
>
> Testing
> -------
>
> Yes
>
>
> Thanks,
>
> Deepak Barr
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Deepak Barr <de...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36133/
-----------------------------------------------------------
(Updated July 28, 2015, 6:58 a.m.)
Review request for lens, Jaideep dhok and Pranav Agarwal.
Changes
-------
Summary of changes :
1. No commit statement for DDL statements.
2. All connection objects will have autocommit=false to encourage good practice for database updates. This makes commit mandatory for any DML statement.
3. Removed create table. A seperate method was not adding any reusability value.
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 (updated)
-----
incubator-lens/lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java 1904350
incubator-lens/lens-server/src/main/java/org/apache/lens/server/util/UtilityMethods.java 9c386a6
Diff: https://reviews.apache.org/r/36133/diff/
Testing
-------
Yes
Thanks,
Deepak Barr
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Deepak Barr <de...@gmail.com>.
> On July 3, 2015, 8:40 a.m., Rajat Khandelwal wrote:
> > lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java, line 9
> > <https://reviews.apache.org/r/36133/diff/1/?file=998310#file998310line9>
> >
> > Please Revert the change
Will fix in the next patch.
- Deepak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36133/#review90321
-----------------------------------------------------------
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
>
>
Re: Review Request 36133: Error creating table finished_queries in
MySQL when autocommit=true
Posted by Rajat Khandelwal <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36133/#review90321
-----------------------------------------------------------
lens-server/src/main/java/org/apache/lens/server/query/LensServerDAO.java (line 9)
<https://reviews.apache.org/r/36133/#comment143340>
Please Revert the change
- Rajat Khandelwal
On July 2, 2015, 8:27 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, 8:27 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
>
>