atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John Speidel <jspei...@hortonworks.com>
Subject Re: Review Request 47382: ATLAS-491 : Business Catalog API
Date Mon, 16 May 2016 19:26:25 GMT


> On May 16, 2016, 5:42 p.m., Madhan Neethiraj wrote:
> > catalog/src/main/java/org/apache/atlas/catalog/BaseResourceProvider.java, line 38
> > <https://reviews.apache.org/r/47382/diff/1/?file=1383415#file1383415line38>
> >
> >     This comment is present in DefaultDateFormatter() as well. Consider removing
from here.

ok


> On May 16, 2016, 5:42 p.m., Madhan Neethiraj wrote:
> > catalog/src/main/java/org/apache/atlas/catalog/DefaultPropertyMapper.java, line
71
> > <https://reviews.apache.org/r/47382/diff/1/?file=1383418#file1383418line71>
> >
> >     for easier readability, consider replacing all "replacement = propName" with
the following block just before the return statement:
> >     
> >       if(replacement == null) {
> >         replacement = propName;
> >       }
> >     
> >       return replacement;

yes, that will make the code cleaner.
I will make the changes.


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

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.


> On May 16, 2016, 5:42 p.m., Madhan Neethiraj wrote:
> > catalog/src/main/java/org/apache/atlas/catalog/DefaultPropertyMapper.java, line
104
> > <https://reviews.apache.org/r/47382/diff/1/?file=1383418#file1383418line104>
> >
> >     Refer to comments for toCleanName() - the same comments apply for toFullyQualifiedName()
as well.

agreed


> On May 16, 2016, 5:42 p.m., Madhan Neethiraj wrote:
> > catalog/src/main/java/org/apache/atlas/catalog/DefaultPropertyMapper.java, line
129
> > <https://reviews.apache.org/r/47382/diff/1/?file=1383418#file1383418line129>
> >
> >     typo: "mnodified_time" ==> "modified_time"

good catch


> On May 16, 2016, 5:42 p.m., Madhan Neethiraj wrote:
> > catalog/src/main/java/org/apache/atlas/catalog/DefaultTypeSystem.java, line 67
> > <https://reviews.apache.org/r/47382/diff/1/?file=1383419#file1383419line67>
> >
> >     Consider adding additional information to the exception - like name/ID of the
existing entity, type & name/ID of entity being created. This will be handly in troubleshooting.
> >     
> >     Please review other exceptions raised in this class for similar updates.

Yeah, there are lots of places where the exception msg could be better.  I just didn't have
the time to get then all cleaned up.  In this case, when I wrote the exception it was my opinion
that because the user know which entity they were creating, and which already exists, that
the message would be sufficient.

I will add additional info to the exception msgs in this class.


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

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> ... "
}


> On May 16, 2016, 5:42 p.m., Madhan Neethiraj wrote:
> > catalog/src/main/java/org/apache/atlas/catalog/BaseResourceProvider.java, line 39
> > <https://reviews.apache.org/r/47382/diff/1/?file=1383415#file1383415line39>
> >
> >     Shouldn't "modified_time" be also registered here?

yes, that makes sense.


> 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().

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?


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

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.


- John


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