> On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/README.md > > Lines 40 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985280#file1985280line40> > > > > bad eol character
fixed, thanks > On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/README.md > > Lines 42 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985280#file1985280line42> > > > > typo sequene > > > > the sentence mentions we and you - I suggest using one or the other. fixed, thanks > On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/README.md > > Lines 58 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985280#file1985280line58> > > > > bad eol character this is for the code to show up propertly in markdown. I will leave like this. > On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/README.md > > Lines 61 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985280#file1985280line61> > > > > I thought impersonation would require a patch on top of Gaian. If this > > is the case we need to detail where to get this patch for impersonation to > > be able to be used here. for now, Nigel add "proxy_user" and "proxy_password" in jdbc url. so there is no patch.Maybe in the future we will change the way we implement impersonation. > On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/README.md > > Lines 69 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985280#file1985280line69> > > > > same this is for the code to show up propertly in markdown. I will leave like this. > On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/README.md > > Lines 71 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985280#file1985280line71> > > > > 'here' is an extra word > > Typo Gian. > > We have not mention LDAP, I think we need to be careful mentioning > > products that might not be installed. > > We should not mention default passwords here - as in production this > > would not make sense. deleted, thanks > On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/README.md > > Lines 79 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985280#file1985280line79> > > > > This text is assuming that people have access to a Gaian Ranger plugin. > > This is not currently available. > > > > You say "it is better to use **table function**. My understanding is > > that using the Ranger plugin requires the table function to be used. > > > > You say "You need to change cst to your own table name". What is cst? for the code now, we need to use table function to work together with Ranger plugin and Gaian. I deleted this totally. > On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/connectors/gaian/GaianOCFConnector.java > > Lines 35 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985285#file1985285line35> > > > > incomplete sentence fixed, thanks > On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/connectors/gaian/GaianOCFConnector.java > > Lines 159 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985285#file1985285line159> > > > > finally is driven for errors as well - so this debug message is not > > correct. I suggst moving this debug to the end of the try section. fixed, thanks > On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/connectors/gaian/GaianOCFConnector.java > > Lines 212 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985285#file1985285line212> > > > > should we return this for an error? i am not quite sure what do you mean > On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/connectors/gaian/GaianOCFConnector.java > > Lines 232 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985285#file1985285line232> > > > > this if and body is a repeat of an previous if. good spot, fixed,thanks > On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/ffdc/OCFDatabaseConnectorErrorCode.java > > Lines 76 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985289#file1985289line76> > > > > what is the user's valid information and valid use of OCF Connectors? user information will be user id. Here should not check user's information. change it to "please check the query and connection to Gaian" > On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/src/main/resources/gaian.properties > > Lines 6 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985292#file1985292line6> > > > > It does not look right to include default passwords here, without > > talking about poc / dev scenarios and not production scenarios for now I am still not sure where do we store the information for the front-end Gaian. For now I just set it up as a properties file. I dont know these password will be stored in a central properties file or also OCF Connection for every asset. I will keep this issue open. > On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/src/test/java/gaianocfconnector/GaianOCFConnectorTest.java > > Lines 56 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985293#file1985293line56> > > > > It looks like this class might a be unit test, but it seems that it > > requires Gaian to be running. If this cannot be run as a unit test - I > > suggest including instructions on how to run the tests in this file. add instructions, thanks > On March 26, 2018, 2 p.m., David Radley wrote: > > ocf-database-connector/src/test/java/gaianocfconnector/UseGaianOCFConnector.java > > Lines 33 (patched) > > <https://reviews.apache.org/r/65435/diff/2/?file=1985294#file1985294line33> > > > > I suggest the test throws the excpetions rather than gobbles them. In > > this way the test fails if there is an error fixed, thanks - Yao ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65435/#review199965 ----------------------------------------------------------- On March 22, 2018, 11:23 a.m., Yao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65435/ > ----------------------------------------------------------- > > (Updated March 22, 2018, 11:23 a.m.) > > > Review request for atlas and Mandy Chessell. > > > Repository: atlas > > > Description > ------- > > This is the new review request for ATLAS-2298 OCF Database Connector. The old > review on > [https://reviews.apache.org/r/65123/](https://reviews.apache.org/r/65123/) > will not be updated anymore. > > The OCF Database Connector is the subclass of OCF Connector and it is > designed especially for connection to database to retrieve data. > Here we implement a connector for Gaian (GaianOCFConnector) as an example for > using OCF Database Connector. It is related to Open Connector Framework. The > JIRA can be found https://issues.apache.org/jira/browse/ATLAS-2298 > > > Diffs > ----- > > ocf-database-connector/README.md PRE-CREATION > ocf-database-connector/pom.xml PRE-CREATION > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/connectors/OCFDatabaseConnection.java > PRE-CREATION > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/connectors/OCFDatabaseConnector.java > PRE-CREATION > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/connectors/OCFDatabaseConnectorProviderBase.java > PRE-CREATION > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/connectors/gaian/GaianOCFConnector.java > PRE-CREATION > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/connectors/gaian/GaianOCFConnectorProvider.java > PRE-CREATION > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/ffdc/DatabaseConnectCheckedException.java > PRE-CREATION > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/ffdc/OCFDatabaseCheckedExceptionBase.java > PRE-CREATION > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/ffdc/OCFDatabaseConnectorErrorCode.java > PRE-CREATION > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/ffdc/OCFDatabaseConnectorRuntimeException.java > PRE-CREATION > > ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/util/PropertiesHelper.java > PRE-CREATION > ocf-database-connector/src/main/resources/gaian.properties PRE-CREATION > > ocf-database-connector/src/test/java/gaianocfconnector/GaianOCFConnectorTest.java > PRE-CREATION > > ocf-database-connector/src/test/java/gaianocfconnector/UseGaianOCFConnector.java > PRE-CREATION > pom.xml ee2746a905f84359d426acfb1fa6d64c6310aaaf > > > Diff: https://reviews.apache.org/r/65435/diff/2/ > > > Testing > ------- > > see test folder. > Gaian has to set up in advance > > > File Attachments > ---------------- > > 0005-ATLAS-2298-05-Feb-code-review.patch > > https://reviews.apache.org/media/uploaded/files/2018/02/05/292c1917-9deb-4ced-8bd6-1689fc45fbd4__0005-ATLAS-2298-05-Feb-code-review.patch > > > Thanks, > > Yao Li > >
