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