Re: Review Request 65123: ATLAS-2298 - Review of OCF JDBC Connector for Gaian

[Yao Li]

> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/JDBCConnection.java
> > Lines 10 (patched)
> > 
> >
> > OMRSConnection --> JDBCConnection

yes fixed, and also change the class name to OCFDatabaseConnection


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/JDBCConnection.java
> > Lines 21 (patched)
> > 
> >
> > OMRSConnection --> JDBCConnection

yes fixed, and also change the class name to OCFDatabaseConnection


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/JDBCConnection.java
> > Lines 84 (patched)
> > 
> >
> > Is it deliberate that we are testing the super-class attribute?

will fix this during the workshop. and this part also show in OMRS Connection.


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/JDBCConnection.java
> > Lines 90 (patched)
> > 
> >
> > Do we need to go to the super class here?

will fix this during the workshop and this part also show in OMRS Connection


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/JDBCConnectorBase.java
> > Lines 32 (patched)
> > 
> >
> > Can the connect() be called multiple times to generate multiple 
> > connections? e.g. with a call to one of the setters between calls to 
> > connect(). I'm just wondering how we'd intend to use the 'conn' member if 
> > that is the case.

the initial design is every time the application wants to execute the query, 
this conn will be get again.


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/JDBCConnectorProviderBase.java
> > Lines 8 (patched)
> > 
> >
> > Should this be an abstract class to force the subclass to implement 
> > this?

yes, fixed this, thanks


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/gaian/GaianJDBCConnector.java
> > Lines 21 (patched)
> > 
> >
> > I would think that host, port, database name, user and password should 
> > all be configurable.

do you mean there should be a seperate configuration file?


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/gaian/GaianJDBCConnector.java
> > Lines 51 (patched)
> > 
> >
> > Beware literal forward-slash characters - they may not work on Windows. 
> > However, this is not always the case - things like the java classloaders 
> > take the forward-slash path separators even on Windows, so it depends on 
> > the specific sub-system we are providing the path/URI to. 
> > It seems that REST URIs and resource file paths (which we will pass to 
> > classloaders) are OK with '/' separators. I am not sure what will work for 
> > the DB connection.

i tested on windows and it works properly. I will keep this issue in mind.


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/gaian/GaianJDBCConnector.java
> > Lines 63 (patched)
> > 
> >
> > You could use the string utils isEmpty() method - or a similar method - 
> > to test for null-ness or emptiness.

use isEmpty() now and in apache commons it offers isNullorEmpty() for String. 
this is also used by Manndy's code, we will talk about this during the workshop


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/gaian/GaianJDBCConnector.java
> > Lines 82 (patched)
> > 
> >
> > Nigel needs to be configurable :-)

haha, fixed. changed it to normal parameter "userId"


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/gaian/GaianJDBCConnector.java
> > Lines 87 (patched)
> > 
> >
> > We shouldn't include System.out.println, but should use log4j (actually 
> > this is a very general comment - lots of logging would be a really nice 
> > addition to this connector generally).

deleted this and add log4j for logging.


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/gaian/GaianJDBCConnector.java
> > Lines 130 (patched)
> > 
> >
> > I think you already renamed ConnectionCheckedException elsewhere? May 
> > want to update the comment

rename the class and updated the comment.


> On Jan. 18, 2018, 12:16 p.m., 

Re: Review Request 65123: ATLAS-2298 - Review of OCF JDBC Connector for Gaian

[Yao Li]

> On Jan. 18, 2018, 12:13 p.m., Mandy Chessell wrote:
> > jdbc/README.md
> > Lines 8 (patched)
> > 
> >
> > should be "... subclass of an OCF Connector"?

yes, fixed, thanks


> On Jan. 18, 2018, 12:13 p.m., Mandy Chessell wrote:
> > jdbc/README.md
> > Lines 9 (patched)
> > 
> >
> > databases

yes, fixed, thanks


> On Jan. 18, 2018, 12:13 p.m., Mandy Chessell wrote:
> > jdbc/README.md
> > Lines 11 (patched)
> > 
> >
> > Need to exmplain the relationship between this JDBC Connector and the 
> > standard JDBC Connector that people are familiar with.

change the name to OCF Database Connector, it will be more clear for people to 
know it is an OCF and for database connection


> On Jan. 18, 2018, 12:13 p.m., Mandy Chessell wrote:
> > jdbc/README.md
> > Lines 17 (patched)
> > 
> >
> > Capitial letter at the start of a sentence.   Also need more context.  
> > For example, what is Gaian?

add introduction to Gaian and purpose for the GaianJDBCConnector


> On Jan. 18, 2018, 12:13 p.m., Mandy Chessell wrote:
> > jdbc/connectors/JDBCConnection.java
> > Lines 8 (patched)
> > 
> >
> > This class name is going to be confusing to people who are familiar 
> > with the JDBC standard.  I think It should have a different name.  Eg 
> > OCFDatabaseConnection

yes, it was confusing when i wrote the code. and now the whole module called 
OCF Database Connector. this class will name as OCFDatabaseConnection


> On Jan. 18, 2018, 12:13 p.m., Mandy Chessell wrote:
> > jdbc/connectors/JDBCConnector.java
> > Lines 10 (patched)
> > 
> >
> > The name of this class is confusing because it is not a JDBCConnector.  
> > JDBC is a standard and this connector does not implement the standard.  So 
> > I would suggest naming it something like OCFDatabaseConnector.

yes, agree with you. now the whole module called OCF Database Connector. this 
class will name as OCFDatabaseConnector


> On Jan. 18, 2018, 12:13 p.m., Mandy Chessell wrote:
> > jdbc/ffdc/ConnectionCheckedException.java
> > Lines 10 (patched)
> > 
> >
> > This is confusing to create a new exception that has the same name as 
> > one of the OCF exceptions.  It should have a different name.

change it to
DatabaseAccessCheckedException extends OCFDatabaseCheckedExceptionBase


> On Jan. 18, 2018, 12:13 p.m., Mandy Chessell wrote:
> > jdbc/ffdc/ExecutionCheckedException.java
> > Lines 14 (patched)
> > 
> >
> > Should this extend JDBCConnectorCheckedExceptionBase?

deleted this one and directly use ConnectorCheckedException from ocf module


- Yao


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65123/#review195714
---


On Jan. 18, 2018, 10:09 a.m., Yao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65123/
> ---
> 
> (Updated Jan. 18, 2018, 10:09 a.m.)
> 
> 
> Review request for atlas and Mandy Chessell.
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> this is the JDBC Connector code for Connector to Gaian. It is related to Open 
> Connector Framework. The JIRA can be found 
> https://issues.apache.org/jira/browse/ATLAS-2298
> 
> 
> Diffs
> -
> 
>   jdbc/README.md PRE-CREATION 
>   jdbc/connectors/JDBCConnection.java PRE-CREATION 
>   jdbc/connectors/JDBCConnector.java PRE-CREATION 
>   jdbc/connectors/JDBCConnectorBase.java PRE-CREATION 
>   jdbc/connectors/JDBCConnectorProviderBase.java PRE-CREATION 
>   jdbc/connectors/gaian/GaianJDBCConnector.java PRE-CREATION 
>   jdbc/connectors/gaian/GaianJDBCConnectorProvider.java PRE-CREATION 
>   jdbc/ffdc/ConnectionCheckedException.java PRE-CREATION 
>   jdbc/ffdc/ExecutionCheckedException.java PRE-CREATION 
>   jdbc/ffdc/JDBCConnectorCheckedExceptionBase.java PRE-CREATION 
>   jdbc/ffdc/JDBCConnectorErrorCode.java PRE-CREATION 
>   jdbc/ffdc/JDBCConnectorRuntimeException.java PRE-CREATION 
>   jdbc/pom.xml PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65123/diff/2/
> 
> 
> Testing
> ---
> 
> create an instance of the Connector and use getData() function. Gaian has to 
> be set up in advance.
> 
> 
> Thanks,
> 
> Yao Li
> 
>

Re: Review Request 65123: ATLAS-2298 - Review of OCF JDBC Connector for Gaian

[Graham Wallis]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65123/#review195710
---




jdbc/connectors/JDBCConnection.java
Lines 10 (patched)


OMRSConnection --> JDBCConnection



jdbc/connectors/JDBCConnection.java
Lines 21 (patched)


OMRSConnection --> JDBCConnection



jdbc/connectors/JDBCConnection.java
Lines 84 (patched)


Is it deliberate that we are testing the super-class attribute?



jdbc/connectors/JDBCConnection.java
Lines 90 (patched)


Do we need to go to the super class here?



jdbc/connectors/JDBCConnectorBase.java
Lines 32 (patched)


Can the connect() be called multiple times to generate multiple 
connections? e.g. with a call to one of the setters between calls to connect(). 
I'm just wondering how we'd intend to use the 'conn' member if that is the case.



jdbc/connectors/JDBCConnectorProviderBase.java
Lines 8 (patched)


Should this be an abstract class to force the subclass to implement this?



jdbc/connectors/gaian/GaianJDBCConnector.java
Lines 21 (patched)


I would think that host, port, database name, user and password should all 
be configurable.



jdbc/connectors/gaian/GaianJDBCConnector.java
Lines 51 (patched)


Beware literal forward-slash characters - they may not work on Windows. 
However, this is not always the case - things like the java classloaders take 
the forward-slash path separators even on Windows, so it depends on the 
specific sub-system we are providing the path/URI to. 
It seems that REST URIs and resource file paths (which we will pass to 
classloaders) are OK with '/' separators. I am not sure what will work for the 
DB connection.



jdbc/connectors/gaian/GaianJDBCConnector.java
Lines 63 (patched)


You could use the string utils isEmpty() method - or a similar method - to 
test for null-ness or emptiness.



jdbc/connectors/gaian/GaianJDBCConnector.java
Lines 82 (patched)


Nigel needs to be configurable :-)



jdbc/connectors/gaian/GaianJDBCConnector.java
Lines 87 (patched)


We shouldn't include System.out.println, but should use log4j (actually 
this is a very general comment - lots of logging would be a really nice 
addition to this connector generally).



jdbc/connectors/gaian/GaianJDBCConnector.java
Lines 130 (patched)


I think you already renamed ConnectionCheckedException elsewhere? May want 
to update the comment



jdbc/connectors/gaian/GaianJDBCConnector.java
Lines 137 (patched)


Should we be throwing the JDBC specific exception class here?



jdbc/connectors/gaian/GaianJDBCConnector.java
Lines 186 (patched)


There are ways to dynamically get the name of the enclosing method - but 
they may have drawbacks (e.g. creation of inner class or additional runtime 
object) - but someone much more expert than me may have a suggestion :-)



jdbc/connectors/gaian/GaianJDBCConnectorProvider.java
Lines 16 (patched)


Should this be extending the JDBC specific base class?



jdbc/ffdc/ConnectionCheckedException.java
Lines 10 (patched)


I thought I saw an earlier review comment where it suggested renaming this 
to JDBCxxx. I might have imagined it, but it does seem that it would be better 
to make this class JDBC specific.



jdbc/ffdc/ExecutionCheckedException.java
Lines 14 (patched)


Should this class also be called something JDBC specific?



jdbc/pom.xml
Lines 6 (patched)


There should be a parent POM reference, which (depending on how nested this 
module is) should look something like this:

apache-atlas
org.apache.atlas
1.0.0-SNAPSHOT

But check whether Mandy wants this nested under
om-fwk-ocf



jdbc/pom.xml
Lines 7 (patched)


The group should probably be "org.apache"



jdbc/pom.xml
Lines 8 (patched)


I suspect artifactId should be something like

Re: Review Request 65123: ATLAS-2298 - Review of OCF JDBC Connector for Gaian

[Mandy Chessell]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65123/#review195714
---




jdbc/README.md
Lines 8 (patched)


should be "... subclass of an OCF Connector"?



jdbc/README.md
Lines 9 (patched)


databases



jdbc/README.md
Lines 11 (patched)


Need to exmplain the relationship between this JDBC Connector and the 
standard JDBC Connector that people are familiar with.



jdbc/README.md
Lines 17 (patched)


Capitial letter at the start of a sentence.   Also need more context.  For 
example, what is Gaian?



jdbc/connectors/JDBCConnection.java
Lines 8 (patched)


This class name is going to be confusing to people who are familiar with 
the JDBC standard.  I think It should have a different name.  Eg 
OCFDatabaseConnection



jdbc/connectors/JDBCConnector.java
Lines 10 (patched)


The name of this class is confusing because it is not a JDBCConnector.  
JDBC is a standard and this connector does not implement the standard.  So I 
would suggest naming it something like OCFDatabaseConnector.



jdbc/ffdc/ConnectionCheckedException.java
Lines 10 (patched)


This is confusing to create a new exception that has the same name as one 
of the OCF exceptions.  It should have a different name.



jdbc/ffdc/ExecutionCheckedException.java
Lines 14 (patched)


Should this extend JDBCConnectorCheckedExceptionBase?


- Mandy Chessell


On Jan. 18, 2018, 10:09 a.m., Yao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65123/
> ---
> 
> (Updated Jan. 18, 2018, 10:09 a.m.)
> 
> 
> Review request for atlas and Mandy Chessell.
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> this is the JDBC Connector code for Connector to Gaian. It is related to Open 
> Connector Framework. The JIRA can be found 
> https://issues.apache.org/jira/browse/ATLAS-2298
> 
> 
> Diffs
> -
> 
>   jdbc/README.md PRE-CREATION 
>   jdbc/connectors/JDBCConnection.java PRE-CREATION 
>   jdbc/connectors/JDBCConnector.java PRE-CREATION 
>   jdbc/connectors/JDBCConnectorBase.java PRE-CREATION 
>   jdbc/connectors/JDBCConnectorProviderBase.java PRE-CREATION 
>   jdbc/connectors/gaian/GaianJDBCConnector.java PRE-CREATION 
>   jdbc/connectors/gaian/GaianJDBCConnectorProvider.java PRE-CREATION 
>   jdbc/ffdc/ConnectionCheckedException.java PRE-CREATION 
>   jdbc/ffdc/ExecutionCheckedException.java PRE-CREATION 
>   jdbc/ffdc/JDBCConnectorCheckedExceptionBase.java PRE-CREATION 
>   jdbc/ffdc/JDBCConnectorErrorCode.java PRE-CREATION 
>   jdbc/ffdc/JDBCConnectorRuntimeException.java PRE-CREATION 
>   jdbc/pom.xml PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65123/diff/2/
> 
> 
> Testing
> ---
> 
> create an instance of the Connector and use getData() function. Gaian has to 
> be set up in advance.
> 
> 
> Thanks,
> 
> Yao Li
> 
>

Re: Review Request 65123: ATLAS-2298 - Review of OCF JDBC Connector for Gaian

[Yao Li]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65123/
---

(Updated Jan. 18, 2018, 10:09 a.m.)


Review request for atlas and Mandy Chessell.


Changes
---

update the code


Repository: atlas


Description
---

this is the JDBC Connector code for Connector to Gaian. It is related to Open 
Connector Framework. The JIRA can be found 
https://issues.apache.org/jira/browse/ATLAS-2298


Diffs (updated)
-

  jdbc/README.md PRE-CREATION 
  jdbc/connectors/JDBCConnection.java PRE-CREATION 
  jdbc/connectors/JDBCConnector.java PRE-CREATION 
  jdbc/connectors/JDBCConnectorBase.java PRE-CREATION 
  jdbc/connectors/JDBCConnectorProviderBase.java PRE-CREATION 
  jdbc/connectors/gaian/GaianJDBCConnector.java PRE-CREATION 
  jdbc/connectors/gaian/GaianJDBCConnectorProvider.java PRE-CREATION 
  jdbc/ffdc/ConnectionCheckedException.java PRE-CREATION 
  jdbc/ffdc/ExecutionCheckedException.java PRE-CREATION 
  jdbc/ffdc/JDBCConnectorCheckedExceptionBase.java PRE-CREATION 
  jdbc/ffdc/JDBCConnectorErrorCode.java PRE-CREATION 
  jdbc/ffdc/JDBCConnectorRuntimeException.java PRE-CREATION 
  jdbc/pom.xml PRE-CREATION 


Diff: https://reviews.apache.org/r/65123/diff/2/

Changes: https://reviews.apache.org/r/65123/diff/1-2/


Testing
---

create an instance of the Connector and use getData() function. Gaian has to be 
set up in advance.


Thanks,

Yao Li