[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1553#issuecomment-220987228
  
ping @swill tag:


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[nlivens]

Github user nlivens commented on the pull request:

https://github.com/apache/cloudstack/pull/1553#issuecomment-220986821
  
Changed db.x.path to db.x.driver as @DaanHoogland suggested


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1553#issuecomment-220983955
  
LGTM though it remains kluncy to some extend I think this is a considerable 
improvement over what we had


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[DaanHoogland]

Github user DaanHoogland commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1553#discussion_r64223844
  
--- Diff: client/tomcatconf/db.properties.in ---
@@ -25,6 +25,7 @@ region.id=1
 db.cloud.username=@DBUSER@
 db.cloud.password=@DBPW@
 db.cloud.host=@DBHOST@
+db.cloud.path=@DBPATH@
--- End diff --

same as above: it contains protocol:driver, not a path


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[nlivens]

Github user nlivens commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1553#discussion_r64223798
  
--- Diff: engine/storage/snapshot/test/resources/db.properties ---
@@ -46,6 +47,8 @@ 
db.cloud.url.params=prepStmtCacheSize=517=true
 db.usage.username=cloud
 db.usage.password=cloud
 db.usage.host=localhost
+# It's not guaranteed that using a different DB provider than the one from 
the regular cloud DB will work
--- End diff --

Yeah, it should work, it's just a matter of using a different driver, but 
unfortunately I'm not able to test such a setup :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[DaanHoogland]

Github user DaanHoogland commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1553#discussion_r64223758
  
--- Diff: build/replace.properties ---
@@ -21,6 +21,7 @@ DBROOTPW=
 MSLOG=vmops.log
 APISERVERLOG=api.log
 DBHOST=localhost
+DBPATH=jdbc:mysql
--- End diff --

maybe protocol/driver instead of path? in fact it contains protocol:driver


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[DaanHoogland]

Github user DaanHoogland commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1553#discussion_r64223544
  
--- Diff: engine/storage/snapshot/test/resources/db.properties ---
@@ -46,6 +47,8 @@ 
db.cloud.url.params=prepStmtCacheSize=517=true
 db.usage.username=cloud
 db.usage.password=cloud
 db.usage.host=localhost
+# It's not guaranteed that using a different DB provider than the one from 
the regular cloud DB will work
--- End diff --

:+1:
of course it should, right ;)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[DaanHoogland]

Github user DaanHoogland commented on the pull request:

https://github.com/apache/cloudstack/pull/1553#issuecomment-220976080
  
@nlivens supakoel


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[nlivens]

Github user nlivens commented on the pull request:

https://github.com/apache/cloudstack/pull/1553#issuecomment-220974849
  
@DaanHoogland, I've added the piece we were talking about, this can be 
found it the logs regarding loading the driver dynamically :
> 2016-05-23 05:42:57,880 DEBUG [c.c.u.d.T.Transaction] 
(localhost-startStop-1:null) (logid:) Successfully loaded DB driver 
com.mysql.jdbc.Driver for connection 
jdbc:mysql://localhost:3306/cloud?autoReconnect=true=517
ePrepStmts=true
> 2016-05-23 05:42:57,896 DEBUG [c.c.u.d.T.Transaction] 
(localhost-startStop-1:null) (logid:) Successfully loaded DB driver 
com.mysql.jdbc.Driver for connection 
jdbc:mysql://localhost:3306/cloud_usage?autoReconnect=true&
> 2016-05-23 05:42:57,898 DEBUG [c.c.u.d.T.Transaction] 
(localhost-startStop-1:null) (logid:) Successfully loaded DB driver 
com.mysql.jdbc.Driver for connection 
jdbc:mysql://localhost:3306/simulator?autoReconnect=true



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[nlivens]

Github user nlivens commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1553#discussion_r64195563
  
--- Diff: framework/db/src/com/cloud/utils/db/TransactionLegacy.java ---
@@ -1014,6 +1014,13 @@ public static void initDataSource(Properties 
dbProps) {
 if (dbProps.size() == 0)
 return;
 
+try {
+// Explicitely load JDBC driver because it has been 
removed from the classpath as part of commit 
c22659d76d73f00f41c13776c490e17a50aacd20
+Class.forName("com.mysql.jdbc.Driver");
--- End diff --

Sure thing, I'll add it to this PR once ready


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[DaanHoogland]

Github user DaanHoogland commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1553#discussion_r64193717
  
--- Diff: framework/db/src/com/cloud/utils/db/TransactionLegacy.java ---
@@ -1014,6 +1014,13 @@ public static void initDataSource(Properties 
dbProps) {
 if (dbProps.size() == 0)
 return;
 
+try {
+// Explicitely load JDBC driver because it has been 
removed from the classpath as part of commit 
c22659d76d73f00f41c13776c490e17a50aacd20
+Class.forName("com.mysql.jdbc.Driver");
--- End diff --

If you can make it work without noticing the time you spend on it, please. 
I would go for db.driver.path btw, so we don't have to maintain a driver table 
in the code.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[nlivens]

Github user nlivens commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1553#discussion_r64191953
  
--- Diff: framework/db/src/com/cloud/utils/db/TransactionLegacy.java ---
@@ -1014,6 +1014,13 @@ public static void initDataSource(Properties 
dbProps) {
 if (dbProps.size() == 0)
 return;
 
+try {
+// Explicitely load JDBC driver because it has been 
removed from the classpath as part of commit 
c22659d76d73f00f41c13776c490e17a50aacd20
+Class.forName("com.mysql.jdbc.Driver");
--- End diff --

Yes, that can be changed to make use of a property value, shall I 
incorporate that change into this PR as well? :)
A property called : "db.x.type" e.g. db.cloud.type=mysql / 
db.cloud.type=mariadb


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[DaanHoogland]

Github user DaanHoogland commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1553#discussion_r64179203
  
--- Diff: framework/db/src/com/cloud/utils/db/TransactionLegacy.java ---
@@ -1014,6 +1014,13 @@ public static void initDataSource(Properties 
dbProps) {
 if (dbProps.size() == 0)
 return;
 
+try {
+// Explicitely load JDBC driver because it has been 
removed from the classpath as part of commit 
c22659d76d73f00f41c13776c490e17a50aacd20
+Class.forName("com.mysql.jdbc.Driver");
--- End diff --

ad 1. so we can use a property that is used for both, right?
ad 2. fair enough, thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[nlivens]

Github user nlivens commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1553#discussion_r64178488
  
--- Diff: framework/db/src/com/cloud/utils/db/TransactionLegacy.java ---
@@ -1014,6 +1014,13 @@ public static void initDataSource(Properties 
dbProps) {
 if (dbProps.size() == 0)
 return;
 
+try {
+// Explicitely load JDBC driver because it has been 
removed from the classpath as part of commit 
c22659d76d73f00f41c13776c490e17a50aacd20
+Class.forName("com.mysql.jdbc.Driver");
--- End diff --

1. You are absolutely right, but I explicitly loaded this one because 
"mysql:jdbc" is hardcoded in the connection URL as well.
2. The problem is that the JAR is actually loaded (so Class.forName works), 
but the JDBC driver is not auto-registering itself. Tomcat has a really 
specific way of auto-registering its JDBC drivers.. 
https://tomcat.apache.org/tomcat-7.0-doc/jndi-datasource-examples-howto.html#DriverManager,_the_service_provider_mechanism_and_memory_leaks
 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[DaanHoogland]

Github user DaanHoogland commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1553#discussion_r64040574
  
--- Diff: framework/db/src/com/cloud/utils/db/TransactionLegacy.java ---
@@ -1014,6 +1014,13 @@ public static void initDataSource(Properties 
dbProps) {
 if (dbProps.size() == 0)
 return;
 
+try {
+// Explicitely load JDBC driver because it has been 
removed from the classpath as part of commit 
c22659d76d73f00f41c13776c490e17a50aacd20
+Class.forName("com.mysql.jdbc.Driver");
--- End diff --

two questions:
hardcoded driver name?; now that we can shouldn't we prepare for a MariaDB 
driver to be loaded instead?
if it is not on the classpath how is this going to help, then? 
Class.forName(..) will search the classpath


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[nlivens]

Github user nlivens commented on the pull request:

https://github.com/apache/cloudstack/pull/1553#issuecomment-220599141
  
@DaanHoogland, triggering you for review as well, this is only a small code 
change


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1553#issuecomment-220547372
  
LGTM, I am aware about the issue and the changes are therefore needed. 
Thanks @nlivens 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[nlivens]

Github user nlivens commented on the pull request:

https://github.com/apache/cloudstack/pull/1553#issuecomment-220546975
  
@swill, @rhtyd, @kiwiflyer, triggering you guys because you were part of 
this mailing thread.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: Explicitely load JDBC driver before creat...

[nlivens]

GitHub user nlivens opened a pull request:

https://github.com/apache/cloudstack/pull/1553

Explicitely load JDBC driver before creating our MySQL connections

Solution to the mailing thread titled "MySQL : No suitable driver found for 
jdbc:mysql".
It doesn't harm that we explicitely load the MySQL driver, and for those 
which would use a commons-dbcp version < 1.4 this would fix it as well. Since 
JDBC 4.0, the JDBC driver can auto-register itself, but for some weird cases 
(like mine), it's not working. Therefore we need to explicitly load the JDBC 
driver.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/nlivens/cloudstack mysql_driver_issue

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/1553.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1553


commit 492c7f2612fdc022d78cd1064ca81e47eb201c04
Author: Nick Livens 
Date:   2016-05-20T08:20:48Z

Explicitely load JDBC driver before creating our MySQL connections




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---