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