Oracle databas supports #387

liningalex · 2017-05-31T23:57:51Z

This pull request is to supports Oracle database. please review. thanks.

ex00 · 2017-06-05T08:27:20Z

storage/jdbc/src/main/scala/org/apache/predictionio/data/storage/ojdbc/StorageClient.scala

+  val settings = ConnectionPoolSettings(maxSize = maxSize)
+
+  ConnectionPool.singleton(
+    config.properties("URL"),


May be do use conts for "URL", "USERNAME", "USERNAME" etc? for avoid problems about copy-paste

changed to extends JDBC StorageClient to avoid code copy.

takezoe · 2017-06-05T16:20:11Z

data/src/main/scala/org/apache/predictionio/data/storage/Storage.scala

+        init.invoke(instance)
+      }
+      catch {
+        case e: NoSuchMethodException => None


If the return value of try expression is not used, Unit is suitable than None for ignoring an exception as following:

case _: NoSuchMethodException => ()

ok, good point.

takezoe · 2017-06-05T16:37:14Z

storage/jdbc/src/main/scala/org/apache/predictionio/data/storage/jdbc/JDBCChannels.scala

@@ -29,14 +29,15 @@ class JDBCChannels(client: String, config: StorageClientConfig, prefix: String)
  extends Channels with Logging {
  /** Database table name for this data access object */
  val tableName = JDBCUtils.prefixTableName(prefix, "channels")
-  DB autoCommit { implicit session =>
-    sql"""
+  def init() {


Don't use the deperecated procedure syntax. See: https://issues.scala-lang.org/browse/SI-7605

It should be:

def init() = { DB autoCommit { implicit session => ... } }

or

def init() = DB autoCommit { implicit session => ... }

takezoe · 2017-06-05T16:39:28Z

storage/jdbc/src/main/scala/org/apache/predictionio/data/storage/jdbc/JDBCEngineInstances.scala

@@ -29,8 +29,9 @@ class JDBCEngineInstances(client: String, config: StorageClientConfig, prefix: S
  extends EngineInstances with Logging {
  /** Database table name for this data access object */
  val tableName = JDBCUtils.prefixTableName(prefix, "engineinstances")
-  DB autoCommit { implicit session =>
-    sql"""
+  def init() {


Same as above.

takezoe · 2017-06-05T16:39:41Z

.../jdbc/src/main/scala/org/apache/predictionio/data/storage/jdbc/JDBCEvaluationInstances.scala

@@ -29,8 +29,9 @@ class JDBCEvaluationInstances(client: String, config: StorageClientConfig, prefi
  extends EvaluationInstances with Logging {
  /** Database table name for this data access object */
  val tableName = JDBCUtils.prefixTableName(prefix, "evaluationinstances")
-  DB autoCommit { implicit session =>
-    sql"""
+  def init() {


Same as above.

takezoe · 2017-06-05T16:39:57Z

storage/jdbc/src/main/scala/org/apache/predictionio/data/storage/jdbc/JDBCModels.scala

-  val binaryColumnType = JDBCUtils.binaryColumnType(client)
-  DB autoCommit { implicit session =>
-    sql"""
+  def init() {


Same as above.

takezoe · 2017-06-05T16:41:11Z

...ge/jdbc/src/main/scala/org/apache/predictionio/data/storage/ojdbc/OJDBCEngineInstances.scala

+/** JDBC implementation of [[EngineInstances]] */
+class OJDBCEngineInstances(client: String, config: StorageClientConfig, prefix: String)
+  extends JDBCEngineInstances(client, config, prefix) {
+  override def init() {


Same as above.

takezoe · 2017-06-05T16:41:23Z

...dbc/src/main/scala/org/apache/predictionio/data/storage/ojdbc/OJDBCEvaluationInstances.scala

+/** JDBC implementations of [[EvaluationInstances]] */
+class OJDBCEvaluationInstances(client: String, config: StorageClientConfig, prefix: String)
+  extends JDBCEvaluationInstances(client, config, prefix) {
+  override def init() {


Same as above.

takezoe · 2017-06-05T16:41:43Z

storage/jdbc/src/main/scala/org/apache/predictionio/data/storage/ojdbc/OJDBCModels.scala

+/** JDBC implementation of [[Models]] */
+class OJDBCModels(client: String, config: StorageClientConfig, prefix: String)
+  extends JDBCModels(client, config, prefix) {
+  override def init() {


Same as above.

takezoe · 2017-06-05T16:50:07Z

data/src/main/scala/org/apache/predictionio/data/storage/Storage.scala

        asInstanceOf[T]
+      try {
+        val init = instance.getClass.getMethod("init")
+        init.invoke(instance)


In my opinion, if init() method is necessary for data objects, we should define a trait (e.g. Initializable) which has a init() method and cast by pattern match here to call init() method.

But since this is just my thought, we need opinion from other committers as well.

takezoe · 2017-06-05T16:54:51Z

core/src/main/scala/org/apache/predictionio/workflow/CreateServer.scala

@@ -198,7 +198,7 @@ object CreateServer extends Logging {
      kryo.invert(modeldata.get(engineInstance.id).get.models).get.
      asInstanceOf[Seq[Any]]

-    val batch = if (engineInstance.batch.nonEmpty) {
+    val batch = if (engineInstance.batch != null && engineInstance.batch.nonEmpty) {


Is this null check necessary?

if not so, pio deploy crashes.

takezoe · 2017-06-05T17:23:57Z

@dszeto As our concern, is it possible to test with Oracle database on Travis-CI or ASF's Jenkins? If no, it might be difficult to guarantee this Oracle support works correctly over the future.

dszeto · 2017-06-08T16:36:08Z

@takezoe A quick search reveals https://hub.docker.com/r/wnameless/oracle-xe-11g/, but this limits the guarantee to whatever Oracle XE 11g supports. @liningalex would you mind taking a look at this option? Let us know if you need help adding integration test.

dszeto · 2017-06-08T16:54:50Z

@liningalex could you also please open a ticket on our Apache JIRA to help with tracking? Thanks!

I will conduct a more thorough review ASAP.

dszeto · 2017-06-16T19:46:37Z

@liningalex Is the main motivation for a separate OJDBC set of classes due to Oracle requiring different column types?

liningalex · 2017-06-16T22:49:40Z

It's lots more than difference of column type.

…

On Fri, Jun 16, 2017 at 12:47 PM, Donald Szeto ***@***.***> wrote: @liningalex <https://github.com/liningalex> Is the main motivation for a separate OJDBC set of classes due to Oracle requiring different column types? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#387 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAneGKyta1GfpUj8kAfOOaPw9qD--gsXks5sEtu-gaJpZM4NsVn1> .

dszeto · 2017-06-20T23:49:07Z

Hey @liningalex, if you don't have a chance adding integration test yet, I would like to merge this to a feature branch so that we can join forces on that.

liningalex · 2017-06-21T22:52:36Z

Hi Donald, I think we should have integration test. do you have instructions how?

…

On Tue, Jun 20, 2017 at 4:49 PM, Donald Szeto ***@***.***> wrote: Hey @liningalex <https://github.com/liningalex>, if you don't have a chance adding integration test yet, I would like to merge this to a feature branch so that we can join forces on that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#387 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAneGNeTQAHMdR346v5vLnQrTTuOcD8Nks5sGFqDgaJpZM4NsVn1> .

dszeto · 2017-07-07T04:39:05Z

We don't have any documentation (which we should add later) on how that's done, but basically, it would be adding a new combination in the test matrix (https://github.com/apache/incubator-predictionio/blob/develop/.travis.yml), and setting up https://github.com/apache/incubator-predictionio/blob/develop/tests/docker-compose.yml to bring up a container running Oracle.

dszeto · 2017-07-26T20:50:07Z

@liningalex Let us know how we can help. This is a great PR and we would like to get it in.

liningalex · 2017-07-27T16:36:17Z

@dszeto is ./run_docker.sh ELASTICSEARCH HBASE LOCALFS ~/Documents/github/incubator-predictionio "echo 'All tests passed...'" right command to run tests? i got

Starting tests_postgres_1 ... done
Starting tests_elasticsearch_1 ... done
Sleeping 30 seconds for all services to be ready...
/init.sh: line 23: ~/Documents/github/incubator-predictionio: No such file or directory

dszeto · 2017-07-27T17:41:35Z

Should be

  tests/run_docker.sh $METADATA_REP $EVENTDATA_REP $MODELDATA_REP \
    "python3 /PredictionIO/tests/pio_tests/tests.py"

where you replace those variables with ORACLE where applicable. This is assuming you have built the Docker image using tests/build_docker.sh.

liningalex · 2017-07-27T19:21:13Z

@dszeto in the end, it says 'Generating XML reports...'. where are the reports?

dszeto · 2017-07-28T18:15:22Z

These reports are generated by XMLTestRunner in tests/pio_tests/tests.py, which are generated inside the container.

liningalex · 2017-07-28T18:42:21Z

@dszeto how do I get the copy?

dszeto · 2017-08-04T17:46:13Z

You will need to use docker commands to either read volumes that were attached to running containers. The test is designed to return non-zero exit code if any tests fail, so if you can run to the end with a zero exit code that would mean tests are passing.

liningalex · 2017-10-04T18:32:29Z

will reopen when integration test is added.

Lining Sun and others added 7 commits May 10, 2017 16:10

Initial cut of Oracle databse supports.

f286a38

Oracle databse supports - extends JDBC classes.

15818cb

Oracle databse supports - add init().

196b661

Oracle databse supports - fixes crashes in pio train and deploy

654aed4

Oracle databse supports - add ORACLE example.

72169b1

oracle databse supports - functionalize ifnotcreate.

1c71b21

Oracle databse supports - add ORACLE example

3479127

ex00 reviewed Jun 5, 2017

View reviewed changes

takezoe suggested changes Jun 5, 2017

View reviewed changes

Oracle databas supports - the code review comments.

a1c68d5

liningalex closed this Oct 4, 2017

Oracle databas supports #387

Oracle databas supports #387

Conversation

liningalex commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takezoe commented Jun 5, 2017

dszeto commented Jun 8, 2017

dszeto commented Jun 8, 2017

dszeto commented Jun 16, 2017

liningalex commented Jun 16, 2017 via email

dszeto commented Jun 20, 2017

liningalex commented Jun 21, 2017 via email

dszeto commented Jul 7, 2017

dszeto commented Jul 26, 2017

liningalex commented Jul 27, 2017

dszeto commented Jul 27, 2017

liningalex commented Jul 27, 2017

dszeto commented Jul 28, 2017

liningalex commented Jul 28, 2017

dszeto commented Aug 4, 2017

liningalex commented Oct 4, 2017