atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Madhan Neethiraj <mad...@apache.org>
Subject Re: Review Request 63503: ATLAS-1095 - Review of OCF - the Open Connector Framework
Date Mon, 04 Dec 2017 20:19:12 GMT

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




om-fwk-ocf/src/main/java/org/apache/atlas/ocf/Connector.java
Lines 60 (patched)
<https://reviews.apache.org/r/63503/#comment270966>

    Instead of defining an interface and a base implementation (Connector & ConnectorBase),
consider having "Connector" as an abstract class. This will allow us to evolve Connector,
for example by adding methods, without having to worry about breaking any implementation of
Connector interface.
    
    With this approach, replace initialize() call with a protected constructor that requires
implementations to supply necessary parameters - like connectorInstanceId and connection.
This will help in not having to expose initialization details to users of Connector instances.
ConnectorProvider.getConnector() implementation should handle creating approriate Connector
implementation with necessary parameters.
    
    public abstract class Connector
    {
        private final String     connectorInstanceId;
        private final Connection connection;
    
        protected Connector(String connectorInstanceId, Connection connection)
        {
            this.connectorInstanceId = connectorInstanceId;
            this.connection          = connection;
        }
        
        public String getConnectorInstanceId() { return connectorInstanceId; }
        
        public Connection getConnection() { return connection; }
    
        // ...
    }



om-fwk-ocf/src/main/java/org/apache/atlas/ocf/Connector.java
Lines 97 (patched)
<https://reviews.apache.org/r/63503/#comment270973>

    If a connector object is specific to an asset, having a set() method would be confusing
and can cause incorrect usage. Instead of set(), asset-properties should be part of initialization.



om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ConnectorBase.java
Lines 53 (patched)
<https://reviews.apache.org/r/63503/#comment270999>

    consider renaming "log" to "LOG" - to be consistent with rest if Atkas sources.



om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ConnectorBroker.java
Lines 70 (patched)
<https://reviews.apache.org/r/63503/#comment271006>

    Consider adding a constructor to Exception class that takes OCFErrorCode as a parameter.
This can help simiplify exception creation, as shown below:
    
      new ConnectionCheckedException(OCFErrorCode.NULL_CONNECTION, this.getClass().getName(),
"getConnector");
      
    
    // constructor
    public ConnectionCheckedException(OCFErrorCode errorCode, className, String actionDescription,
Object...errorMessageParams) {
      this(errorCode.getHTTPErrorCode(), className, actionDescription, errorCode.getErrorMessageId()
+ errorCode.getFormattedErrorMessage(errorMessageParams), errorCode.getSystemAction(), errorCode.getUserAction());
    }
    
    Consider the same approach for other exceptions as well - like ConnectorCheckedException,
OCFRuntimeException



om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ConnectorProvider.java
Lines 37 (patched)
<https://reviews.apache.org/r/63503/#comment270978>

    Similar to earlier comment on Connector & ConnectorBase (to replace interface and
base implementation with an abstract class), consider changing ConnectionProvider interface
to an abstract class.



om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ffdc/OCFErrorCode.java
Lines 183 (patched)
<https://reviews.apache.org/r/63503/#comment271008>

    Consider dropping prefix "new" in the parameter names - here is other occurances.



om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ffdc/OCFErrorCode.java
Lines 227 (patched)
<https://reviews.apache.org/r/63503/#comment271010>

    Consider changing "String... params" to "Object... params". This will simplify callers
in not having to call ".toString()" on parameters (and check for null before calling .toString())



om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/AnnotationStatus.java
Lines 38 (patched)
<https://reviews.apache.org/r/63503/#comment271014>

    Consider marking these members as final.


- Madhan Neethiraj


On Nov. 10, 2017, 12:48 p.m., Mandy Chessell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63503/
> -----------------------------------------------------------
> 
> (Updated Nov. 10, 2017, 12:48 p.m.)
> 
> 
> Review request for atlas and Madhan Neethiraj.
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> This patch contains the open connector framework code.  This code is located in the om-fwk-ocf
component and is described in JIRA https://issues.apache.org/jira/browse/ATLAS-1095
> 
> I have added a new patch to the Jira with fixes from Yao's comments.  Upgraded Maven
to 352 and rebuilt/rerun tests.
> 
> 
> Diffs
> -----
> 
>   om-fwk-ocf/README.md PRE-CREATION 
>   om-fwk-ocf/pom.xml PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/Connector.java PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ConnectorBase.java PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ConnectorBroker.java PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ConnectorProvider.java PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ConnectorProviderBase.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ffdc/ConnectionCheckedException.java
PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ffdc/ConnectorCheckedException.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ffdc/OCFCheckedExceptionBase.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ffdc/OCFErrorCode.java PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ffdc/OCFRuntimeException.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ffdc/PropertyServerException.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/ffdc/README.md PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/AdditionalProperties.java
PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Analysis.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Annotation.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/AnnotationStatus.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Annotations.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/AssetDescriptor.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/AssetDetail.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/AssetPropertyBase.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/AssetSummary.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/AssetUniverse.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Certification.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Certifications.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Classification.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Classifications.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Comment.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/CommentType.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Comments.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/ConnectedAssetProperties.java
PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Connection.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Connections.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/ConnectorType.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/DerivedSchemaElement.java
PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/ElementHeader.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/ElementType.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/EmbeddedConnection.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/EmbeddedConnections.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Endpoint.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/ExternalIdentifier.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/ExternalIdentifiers.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/ExternalReference.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/ExternalReferences.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Feedback.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/InformalTag.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/InformalTags.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/KeyPattern.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/License.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Licenses.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Like.java PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Likes.java PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Lineage.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Location.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Locations.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/MapSchemaElement.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Meaning.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Meanings.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Note.java PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/NoteLog.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/NoteLogs.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Notes.java PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/PrimitiveSchemaElement.java
PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/PropertyBase.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Rating.java PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Ratings.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Referenceable.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/RelatedAsset.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/RelatedAssetProperties.java
PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/RelatedAssets.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/RelatedMediaReference.java
PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/RelatedMediaReferences.java
PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/RelatedMediaType.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/RelatedMediaUsage.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/Schema.java PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/SchemaAttribute.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/SchemaAttributeGUIDs.java
PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/SchemaAttributes.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/SchemaElement.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/SchemaImplementationQueries.java
PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/SchemaImplementationQuery.java
PRE-CREATION 
>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/SchemaLink.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/SchemaLinks.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/StarRating.java PRE-CREATION

>   om-fwk-ocf/src/main/java/org/apache/atlas/ocf/properties/VirtualConnection.java PRE-CREATION

>   om-fwk-ocf/src/test/java/org/apache/atlas/ocf/test/QuickTest.java PRE-CREATION 
>   om-fwk-ocf/src/test/java/org/apache/atlas/ocf/test/TestConnector.java PRE-CREATION

>   om-fwk-ocf/src/test/java/org/apache/atlas/ocf/test/TestConnectorProvider.java PRE-CREATION

> 
> 
> Diff: https://reviews.apache.org/r/63503/diff/2/
> 
> 
> Testing
> -------
> 
> Simple sniff test to create connectors.
> 
> 
> File Attachments
> ----------------
> 
> 0001-ATLAS-1095-initial-code-drop-for-OCF-with-fixes-from.patch
>   https://reviews.apache.org/media/uploaded/files/2017/11/10/ad9eae3c-bb68-4bdc-b6b9-62ba12f651a0__0001-ATLAS-1095-initial-code-drop-for-OCF-with-fixes-from.patch
> 
> 
> Thanks,
> 
> Mandy Chessell
> 
>


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