atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Radley <david...@apache.org>
Subject Re: Review Request 65435: ATLAS-2298 - Review of OCF Database Connector_New
Date Mon, 26 Mar 2018 14:00:48 GMT

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




ocf-database-connector/README.md
Lines 40 (patched)
<https://reviews.apache.org/r/65435/#comment280469>

    bad eol character



ocf-database-connector/README.md
Lines 42 (patched)
<https://reviews.apache.org/r/65435/#comment280472>

    typo sequene
    
    the sentence mentions we and you - I suggest using one or the other.



ocf-database-connector/README.md
Lines 58 (patched)
<https://reviews.apache.org/r/65435/#comment280470>

    bad eol character



ocf-database-connector/README.md
Lines 61 (patched)
<https://reviews.apache.org/r/65435/#comment280474>

    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.



ocf-database-connector/README.md
Lines 69 (patched)
<https://reviews.apache.org/r/65435/#comment280471>

    same



ocf-database-connector/README.md
Lines 71 (patched)
<https://reviews.apache.org/r/65435/#comment280473>

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



ocf-database-connector/README.md
Lines 79 (patched)
<https://reviews.apache.org/r/65435/#comment280475>

    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?



ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/connectors/gaian/GaianOCFConnector.java
Lines 35 (patched)
<https://reviews.apache.org/r/65435/#comment280477>

    incomplete sentence



ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/connectors/gaian/GaianOCFConnector.java
Lines 159 (patched)
<https://reviews.apache.org/r/65435/#comment280478>

    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.



ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/connectors/gaian/GaianOCFConnector.java
Lines 212 (patched)
<https://reviews.apache.org/r/65435/#comment280479>

    should we return this for an error?



ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/connectors/gaian/GaianOCFConnector.java
Lines 232 (patched)
<https://reviews.apache.org/r/65435/#comment280480>

    this if and body is a repeat of an previous if.



ocf-database-connector/src/main/java/org/apache/atlas/ocfdbconnector/ffdc/OCFDatabaseConnectorErrorCode.java
Lines 76 (patched)
<https://reviews.apache.org/r/65435/#comment280481>

    what is the user's valid information and valid use of OCF Connectors?



ocf-database-connector/src/main/resources/gaian.properties
Lines 6 (patched)
<https://reviews.apache.org/r/65435/#comment280482>

    It does not look right to include default passwords here, without talking about poc /
dev scenarios and not production scenarios



ocf-database-connector/src/test/java/gaianocfconnector/GaianOCFConnectorTest.java
Lines 56 (patched)
<https://reviews.apache.org/r/65435/#comment280483>

    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.



ocf-database-connector/src/test/java/gaianocfconnector/UseGaianOCFConnector.java
Lines 33 (patched)
<https://reviews.apache.org/r/65435/#comment280484>

    I suggest the test throws the excpetions rather than gobbles them. In this way the test
fails if there is an error


- David Radley


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


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