----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65123/#review195710 -----------------------------------------------------------
jdbc/connectors/JDBCConnection.java Lines 10 (patched) <https://reviews.apache.org/r/65123/#comment274976> OMRSConnection --> JDBCConnection jdbc/connectors/JDBCConnection.java Lines 21 (patched) <https://reviews.apache.org/r/65123/#comment274977> OMRSConnection --> JDBCConnection jdbc/connectors/JDBCConnection.java Lines 84 (patched) <https://reviews.apache.org/r/65123/#comment274978> Is it deliberate that we are testing the super-class attribute? jdbc/connectors/JDBCConnection.java Lines 90 (patched) <https://reviews.apache.org/r/65123/#comment274979> Do we need to go to the super class here? jdbc/connectors/JDBCConnectorBase.java Lines 32 (patched) <https://reviews.apache.org/r/65123/#comment274981> 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) <https://reviews.apache.org/r/65123/#comment274982> Should this be an abstract class to force the subclass to implement this? jdbc/connectors/gaian/GaianJDBCConnector.java Lines 21 (patched) <https://reviews.apache.org/r/65123/#comment274983> I would think that host, port, database name, user and password should all be configurable. jdbc/connectors/gaian/GaianJDBCConnector.java Lines 51 (patched) <https://reviews.apache.org/r/65123/#comment274985> 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) <https://reviews.apache.org/r/65123/#comment274986> 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) <https://reviews.apache.org/r/65123/#comment274984> Nigel needs to be configurable :-) jdbc/connectors/gaian/GaianJDBCConnector.java Lines 87 (patched) <https://reviews.apache.org/r/65123/#comment274987> 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) <https://reviews.apache.org/r/65123/#comment274988> I think you already renamed ConnectionCheckedException elsewhere? May want to update the comment jdbc/connectors/gaian/GaianJDBCConnector.java Lines 137 (patched) <https://reviews.apache.org/r/65123/#comment274989> Should we be throwing the JDBC specific exception class here? jdbc/connectors/gaian/GaianJDBCConnector.java Lines 186 (patched) <https://reviews.apache.org/r/65123/#comment274991> 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) <https://reviews.apache.org/r/65123/#comment275002> Should this be extending the JDBC specific base class? jdbc/ffdc/ConnectionCheckedException.java Lines 10 (patched) <https://reviews.apache.org/r/65123/#comment275003> 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) <https://reviews.apache.org/r/65123/#comment275004> Should this class also be called something JDBC specific? jdbc/pom.xml Lines 6 (patched) <https://reviews.apache.org/r/65123/#comment274974> 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> jdbc/pom.xml Lines 7 (patched) <https://reviews.apache.org/r/65123/#comment274973> The group should probably be "org.apache" jdbc/pom.xml Lines 8 (patched) <https://reviews.apache.org/r/65123/#comment274975> I suspect artifactId should be something like <artifactId>atlas-ocf-jdbc</artifactId> depending on nesting of modules. jdbc/pom.xml Lines 9 (patched) <https://reviews.apache.org/r/65123/#comment274980> I think version should be 1.0.0-SNAPSHOT - Graham Wallis 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 > >
