atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yao Li <bertha...@126.com>
Subject Re: Review Request 65123: ATLAS-2298 - Review of OCF JDBC Connector for Gaian
Date Mon, 29 Jan 2018 15:19:54 GMT


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/JDBCConnection.java
> > Lines 10 (patched)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941456#file1941456line10>
> >
> >     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)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941456#file1941456line21>
> >
> >     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)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941456#file1941456line84>
> >
> >     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)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941456#file1941456line90>
> >
> >     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)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941458#file1941458line32>
> >
> >     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)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941459#file1941459line8>
> >
> >     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)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941460#file1941460line21>
> >
> >     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)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941460#file1941460line51>
> >
> >     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)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941460#file1941460line63>
> >
> >     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)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941460#file1941460line82>
> >
> >     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)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941460#file1941460line87>
> >
> >     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)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941460#file1941460line130>
> >
> >     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., Graham Wallis wrote:
> > jdbc/connectors/gaian/GaianJDBCConnector.java
> > Lines 137 (patched)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941460#file1941460line137>
> >
> >     Should we be throwing the JDBC specific exception class here?

here if we receive the SQL exception, we will throw our own ocf database exception. I want
to wrap it to our own exception.


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/gaian/GaianJDBCConnector.java
> > Lines 186 (patched)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941460#file1941460line186>
> >
> >     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 :-)

will talk about this through the workshop. thanks


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/connectors/gaian/GaianJDBCConnectorProvider.java
> > Lines 16 (patched)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941461#file1941461line16>
> >
> >     Should this be extending the JDBC specific base class?

the old nameing here is very confusing. for new patch I changed all names to OCFDatabaseXXXX,
here this class changed to GaianOCFConnectorProvider.


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/ffdc/ConnectionCheckedException.java
> > Lines 10 (patched)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941462#file1941462line10>
> >
> >     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.

the old nameing here is very confusing. for new patch I changed all names to OCFDatabaseXXXX.
the most import concept here is it is an OCF complaint connector.


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/ffdc/ExecutionCheckedException.java
> > Lines 14 (patched)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941463#file1941463line14>
> >
> >     Should this class also be called something JDBC specific?

the old nameing here is very confusing. for new patch I changed all names to OCFDatabaseXXXX.
this module is an OCF connector but expecially for database.


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/pom.xml
> > Lines 6 (patched)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941467#file1941467line6>
> >
> >     There should be a parent POM reference, which (depending on how nested this
module is) should look something like this:
> >     <parent>
> >             <artifactId>apache-atlas</artifactId>
> >             <groupId>org.apache.atlas</groupId>
> >             <version>1.0.0-SNAPSHOT</version>
> >     </parent>
> >     But check whether Mandy wants this nested under
> >     <artifactId>om-fwk-ocf</artifactId>

will fix this during the workshop on 30-01. thanks!


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/pom.xml
> > Lines 7 (patched)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941467#file1941467line7>
> >
> >     The group should probably be "org.apache"

yes, fixed.


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/pom.xml
> > Lines 8 (patched)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941467#file1941467line8>
> >
> >     I suspect artifactId should be something like
> >     <artifactId>atlas-ocf-jdbc</artifactId>
> >     depending on nesting of modules.

change it to:

    <parent>
        <artifactId>apache-atlas</artifactId>
        <groupId>org.apache.atlas</groupId>
        <version>1.0.0-SNAPSHOT</version>
    </parent>
    <artifactId>atlas-ocf-database</artifactId>
    <name>OCF Database Connector</name>
    <description>OCF Database Connector provides the interfaces and implementation of
Connectors for retrieving data from databases.</description>

    <packaging>jar</packaging>


> On Jan. 18, 2018, 12:16 p.m., Graham Wallis wrote:
> > jdbc/pom.xml
> > Lines 9 (patched)
> > <https://reviews.apache.org/r/65123/diff/2/?file=1941467#file1941467line9>
> >
> >     I think version should be 1.0.0-SNAPSHOT

yes, fixed, thanks


- Yao


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


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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message