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 47382: ATLAS-491 : Business Catalog API
Date Mon, 16 May 2016 23:52:13 GMT


> On May 16, 2016, 5:42 p.m., Madhan Neethiraj wrote:
> > catalog/src/main/java/org/apache/atlas/catalog/DefaultPropertyMapper.java, line
80
> > <https://reviews.apache.org/r/47382/diff/1/?file=1383418#file1383418line80>
> >
> >     Would it help to update m_qualifiedToCleanMap with the result? The next lookup
would be faster. This would work only if dataType.fieldMapping().fields remains unchanged.
> >     
> >     m_qualifiedToCleanMap.put(propName, replacement);
> 
> John Speidel wrote:
>     yeah, I thought about that but it seemed like a premature optimization that would
cause issues if the underlying mappings changed as you mentioned.
>     IMO, there are a lot of other places where we could optimize before this.

ok, thanks. We can revisit this in a later patch.


> On May 16, 2016, 5:42 p.m., Madhan Neethiraj wrote:
> > catalog/src/main/java/org/apache/atlas/catalog/EntityResourceProvider.java, line
47
> > <https://reviews.apache.org/r/47382/diff/1/?file=1383420#file1383420line47>
> >
> >     Would request have ID-property in all cases? If not, consider adding request.toString()
to the exception message. AtlasQuery implementations should return appropriate value from
toString().
> 
> John Speidel wrote:
>     Yes, the request would fail validation if the id isn't provided so the request will
always contain the ID property at this point.
>     I agree that it would be nice to provide a user friendly toString() implementation
to AtlasQuery implementations, unfortunately the toString() provided by GremlinPipeline isn't
very user friendly so a bit of work will be required to provide this.  Would it be ok to handle
this as a separate follow-up Jira?

Yes. Review/update of the exception message text can be handled in a separate JIRA.


> On May 16, 2016, 5:42 p.m., Madhan Neethiraj wrote:
> > catalog/src/main/java/org/apache/atlas/catalog/ResourceProvider.java, line 37
> > <https://reviews.apache.org/r/47382/diff/1/?file=1383428#file1383428line37>
> >
> >     Since this method requires ID to be given, consider renaming this to getResourceById().
> >     
> >     Also consider if making the return type to "SingleResult" (a new class) would
make it easier to read/understand. "Result" deals with a Collection of propertyMaps.
> 
> John Speidel wrote:
>     ok, I will rename the method.
>     Yes, it would probably be cleaner to return a result specific to single results.
>     Due to time considerations, this patch is being offered for review in a state that
isn't consistent with my normal standards for a patch being posted for review.  So, there
are many refactorings and other cleanup that I would like to do but just didn't/don't have
the time.

ok. Method renaming is good for this patch. Lets review on using a different return type for
getResourceById() in a separate JIRA.


> On May 16, 2016, 5:42 p.m., Madhan Neethiraj wrote:
> > catalog/src/main/java/org/apache/atlas/catalog/TaxonomyResourceProvider.java, line
41
> > <https://reviews.apache.org/r/47382/diff/1/?file=1383430#file1383430line41>
> >
> >     Consider adding query details in the exception message to help in troubleshooting.
> 
> John Speidel wrote:
>     Parsing errors are already provied to the user in this case:
>     For the invalid query: api/atlas/v1/entities?someProperty~~InvalidOperator~~someValue
>     
>     The following response is returned:
>     
>     {
>     status: "400",
>     message: "Unable to parse query: Cannot parse 'someProperty~~InvalidOperator~~someValue':
Encountered " <FUZZY_SLOP> "~InvalidOperator "" at line 1, column 13. Was expecting
one of: <EOF> <AND> ... <OR> ... <NOT> ... "+" ... "-" ... <BAREOPER>
... "(" ... "*" ... "^" ... <QUOTED> ... <TERM> ... <PREFIXTERM> ... <WILDTERM>
... <REGEXPTERM> ... "[" ... "{" ... <NUMBER> ... "
>     }

ok. These details look good for troubleshooting. No changes are needed. Thanks.


- Madhan


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


On May 14, 2016, 12:58 a.m., John Speidel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47382/
> -----------------------------------------------------------
> 
> (Updated May 14, 2016, 12:58 a.m.)
> 
> 
> Review request for atlas and Hemanth Yamijala.
> 
> 
> Bugs: ATLAS-491
>     https://issues.apache.org/jira/browse/ATLAS-491
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Initial implementation of Catalog API.
> 
> Known Issues/Limitations:
> - No update/delete support for taxonomies/terms
>   -- this will be added next week
> - Some gaps in test coverage
>   - Need more in-depth test coverage that exercise the generated gremlin pipeline
> - some refactoring should still be done espcecially in the 'query' package
> - need to add more logging
> - need to finish some javadoc
> 
> 
> Diffs
> -----
> 
>   catalog/pom.xml PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/AtlasTypeSystem.java PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/BaseRequest.java PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/BaseResourceProvider.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/CollectionRequest.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/DefaultDateFormatter.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/DefaultPropertyMapper.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/DefaultTypeSystem.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/EntityResourceProvider.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/EntityTagResourceProvider.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/InstanceRequest.java PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/JsonSerializer.java PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/PropertyMapper.java PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/PropertyValueFormatter.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/Request.java PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/ResourceComparator.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/ResourceProvider.java PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/Result.java PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/TaxonomyResourceProvider.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/TermPath.java PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/TermResourceProvider.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/TermVertexWrapper.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/VertexWrapper.java PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/definition/BaseResourceDefinition.java
PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/definition/EntityResourceDefinition.java
PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/definition/EntityTagResourceDefinition.java
PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/definition/ResourceDefinition.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/definition/TaxonomyResourceDefinition.java
PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/definition/TermResourceDefinition.java
PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/exception/CatalogException.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/exception/CatalogRuntimeException.java
PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/exception/InvalidPayloadException.java
PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/exception/InvalidQueryException.java
PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/exception/ResourceAlreadyExistsException.java
PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/exception/ResourceNotFoundException.java
PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/projection/GenericRelation.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/projection/Projection.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/projection/ProjectionResult.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/projection/Relation.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/projection/RelationProjection.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/projection/RelationSet.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/projection/TagRelation.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/projection/TraitRelation.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/query/AlwaysQueryExpression.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/query/AtlasEntityQuery.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/query/AtlasEntityTagQuery.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/query/AtlasQuery.java PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/query/AtlasTaxonomyQuery.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/query/AtlasTermQuery.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/query/BaseQuery.java PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/query/BaseQueryExpression.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/query/BooleanQueryExpression.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/query/PrefixQueryExpression.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/query/ProjectionQueryExpression.java
PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/query/QueryExpression.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/query/QueryFactory.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/query/RegexQueryExpression.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/query/TermQueryExpression.java PRE-CREATION

>   catalog/src/main/java/org/apache/atlas/catalog/query/TermRangeQueryExpression.java
PRE-CREATION 
>   catalog/src/main/java/org/apache/atlas/catalog/query/WildcardQueryExpression.java PRE-CREATION

>   catalog/src/test/java/org/apache/atlas/catalog/CollectionRequestTest.java PRE-CREATION

>   catalog/src/test/java/org/apache/atlas/catalog/DefaultDateFormatterTest.java PRE-CREATION

>   catalog/src/test/java/org/apache/atlas/catalog/DefaultPropertyMapperTest.java PRE-CREATION

>   catalog/src/test/java/org/apache/atlas/catalog/EntityResourceProviderTest.java PRE-CREATION

>   catalog/src/test/java/org/apache/atlas/catalog/EntityTagResourceProviderTest.java PRE-CREATION

>   catalog/src/test/java/org/apache/atlas/catalog/InstanceRequestTest.java PRE-CREATION

>   catalog/src/test/java/org/apache/atlas/catalog/JsonSerializerTest.java PRE-CREATION

>   catalog/src/test/java/org/apache/atlas/catalog/ResourceComparatorTest.java PRE-CREATION

>   catalog/src/test/java/org/apache/atlas/catalog/TaxonomyResourceProviderTest.java PRE-CREATION

>   catalog/src/test/java/org/apache/atlas/catalog/TermResourceProviderTest.java PRE-CREATION

>   catalog/src/test/java/org/apache/atlas/catalog/VertexWrapperTest.java PRE-CREATION

>   catalog/src/test/java/org/apache/atlas/catalog/definition/EntityResourceDefinitionTest.java
PRE-CREATION 
>   catalog/src/test/java/org/apache/atlas/catalog/definition/EntityTagResourceDefinitionTest.java
PRE-CREATION 
>   catalog/src/test/java/org/apache/atlas/catalog/definition/TaxonomyResourceDefinitionTest.java
PRE-CREATION 
>   catalog/src/test/java/org/apache/atlas/catalog/definition/TermResourceDefinitionTest.java
PRE-CREATION 
>   catalog/src/test/java/org/apache/atlas/catalog/query/AlwaysQueryExpressionTest.java
PRE-CREATION 
>   catalog/src/test/java/org/apache/atlas/catalog/query/QueryFactoryTest.java PRE-CREATION

>   pom.xml 5e2871e 
>   repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java cccafc2

>   repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java 5195cbe

>   server-api/src/main/java/org/apache/atlas/services/MetadataService.java 13d20d8 
>   typesystem/src/main/java/org/apache/atlas/typesystem/persistence/StructInstance.java
6fb2087 
>   typesystem/src/main/java/org/apache/atlas/typesystem/types/TypedStructHandler.java
b97669a 
>   webapp/pom.xml de48c15 
>   webapp/src/main/java/org/apache/atlas/web/resources/BaseService.java PRE-CREATION 
>   webapp/src/main/java/org/apache/atlas/web/resources/CatalogExceptionMapper.java PRE-CREATION

>   webapp/src/main/java/org/apache/atlas/web/resources/CatalogRuntimeExceptionMapper.java
PRE-CREATION 
>   webapp/src/main/java/org/apache/atlas/web/resources/EntityService.java PRE-CREATION

>   webapp/src/main/java/org/apache/atlas/web/resources/TaxonomyService.java PRE-CREATION

>   webapp/src/main/webapp/WEB-INF/web.xml f0b606e 
> 
> Diff: https://reviews.apache.org/r/47382/diff/
> 
> 
> Testing
> -------
> 
> Ran all existing unit tests and added new tests.
> 
> 
> Thanks,
> 
> John Speidel
> 
>


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