> On May 16, 2016, 11:53 a.m., Shwetha GS wrote: > > common/src/main/java/org/apache/atlas/AtlasRuntimeException.java, line 23 > > <https://reviews.apache.org/r/46330/diff/4/?file=1364717#file1364717line23> > > > > Why throw unchecked exception? Should be checked exception so that > > usages can handle it
Agreed. The reason I chose to initially to go with a subclass of RuntimeException is to avoid refactoring a whole bunch of methods making calls to TypeSystem API, and further call stack. If you feel strongly about having to use checked exception, I can make the changes. > On May 16, 2016, 11:53 a.m., Shwetha GS wrote: > > repository/src/main/java/org/apache/atlas/RepositoryMetadataModule.java, > > line 135 > > <https://reviews.apache.org/r/46330/diff/4/?file=1364719#file1364719line135> > > > > Use ApplicationProperties.getClass() The initialization of cache provider has now been moved from TypeSystemProvider to the constructor of TypeSystem and the ApplicationProperties.getClass() has been used to get hold of the cache provider. > On May 16, 2016, 11:53 a.m., Shwetha GS wrote: > > typesystem/src/main/java/org/apache/atlas/typesystem/types/TypeSystem.java, > > line 167 > > <https://reviews.apache.org/r/46330/diff/4/?file=1364725#file1364725line167> > > > > should also check among core types Good find. Done. > On May 16, 2016, 11:53 a.m., Shwetha GS wrote: > > typesystem/src/main/java/org/apache/atlas/typesystem/types/TypeSystem.java, > > line 328 > > <https://reviews.apache.org/r/46330/diff/4/?file=1364725#file1364725line328> > > > > use isRegistered() Good one. All internal type lookup with typeCache.has has been modified to use isRegistered. > On May 16, 2016, 11:53 a.m., Shwetha GS wrote: > > typesystem/src/main/java/org/apache/atlas/typesystem/types/TypeSystem.java, > > line 431 > > <https://reviews.apache.org/r/46330/diff/4/?file=1364725#file1364725line431> > > > > Use isRegistered() wherever typeCache.has() is used Done. > On May 16, 2016, 11:53 a.m., Shwetha GS wrote: > > typesystem/src/main/java/org/apache/atlas/typesystem/types/TypeSystemProvider.java, > > line 44 > > <https://reviews.apache.org/r/46330/diff/4/?file=1364726#file1364726line44> > > > > It doesn't make sense to create an instance of TypeSystem without > > setCacheProvider(). Can you move initialising cache provider to TypeSystem > > constructor? Completely agreed. This was something which I struggled to make a good choice between using Guice versus and not using it, especially considering the TypeSystem bypasses the Guice by way of being directly accessed in all the dependencies with TypeSystem.getInstance. Also, once a user calls TypeSystem.getInstance, a completely ready instance of TypeSystem should be returned. But the instance of TypeSystem is created privately within TypeSystem to honor singleton pattern (infact ideally the constructor should have been declared private, but has been made public probably for testing purposes). So, I finally moved the cache provider initialization to the TypeSystem::initialize. > On May 16, 2016, 11:53 a.m., Shwetha GS wrote: > > typesystem/src/main/java/org/apache/atlas/typesystem/types/cache/DefaultTypeCacheProvider.java, > > line 64 > > <https://reviews.apache.org/r/46330/diff/4/?file=1364727#file1364727line64> > > > > I don't see any usages of this except in tests, can you remove this > > interface? This was added to later address to further tweak the calls to cache provider from TypeSystem. In the sense, an implementer can chose to provide a better performing cache provider if the types can be partitioned based on their category. TypeSystem code, at many places, has the type category info, so there is a chance for further refactoring those call points and the DefaultTypeCacheProvider. I know it is not needed to code for future, but we plan to do that soon after this JIRA goes through. > On May 16, 2016, 11:53 a.m., Shwetha GS wrote: > > typesystem/src/main/java/org/apache/atlas/typesystem/types/cache/DefaultTypeCacheProvider.java, > > line 110 > > <https://reviews.apache.org/r/46330/diff/4/?file=1364727#file1364727line110> > > > > Not required? Please see the comment above. > On May 16, 2016, 11:53 a.m., Shwetha GS wrote: > > typesystem/src/main/java/org/apache/atlas/typesystem/types/cache/DefaultTypeCacheProvider.java, > > line 124 > > <https://reviews.apache.org/r/46330/diff/4/?file=1364727#file1364727line124> > > > > Faster to maintain another map like earlier? Agreed. Please see my earlier comment (2 comments above). The plan is to further refactor calls made to type cache to lookup more narrowly based on category. As part, this comment should get addressed. > On May 16, 2016, 11:53 a.m., Shwetha GS wrote: > > typesystem/src/main/java/org/apache/atlas/typesystem/types/cache/DefaultTypeCacheProvider.java, > > line 125 > > <https://reviews.apache.org/r/46330/diff/4/?file=1364727#file1364727line125> > > > > Rename getNames() to getTypeNames() Done. > On May 16, 2016, 11:53 a.m., Shwetha GS wrote: > > typesystem/src/main/java/org/apache/atlas/typesystem/types/cache/DefaultTypeCacheProvider.java, > > line 134 > > <https://reviews.apache.org/r/46330/diff/4/?file=1364727#file1364727line134> > > > > Check for !coreTypes? Good find. This check is no more necessary and I have removed it. I had added it very early when the type cache was even persisting coretypes accidentally. As the core types are fixed for the system, they do not need to go to type cache provider. > On May 16, 2016, 11:53 a.m., Shwetha GS wrote: > > typesystem/src/main/java/org/apache/atlas/typesystem/types/cache/DefaultTypeCacheProvider.java, > > line 151 > > <https://reviews.apache.org/r/46330/diff/4/?file=1364727#file1364727line151> > > > > Rename to getAllTypeNames() Done > On May 16, 2016, 11:53 a.m., Shwetha GS wrote: > > typesystem/src/main/java/org/apache/atlas/typesystem/types/cache/ITypeCacheProvider.java, > > line 133 > > <https://reviews.apache.org/r/46330/diff/4/?file=1364728#file1364728line133> > > > > no params Great find :-) - venkata ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46330/#review133344 ----------------------------------------------------------- On May 17, 2016, 2:55 p.m., venkata madugundu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46330/ > ----------------------------------------------------------- > > (Updated May 17, 2016, 2:55 p.m.) > > > Review request for atlas, David Kantor and Hemanth Yamijala. > > > Bugs: ATLAS-683 > https://issues.apache.org/jira/browse/ATLAS-683 > > > Repository: atlas > > > Description > ------- > > The local type-cache has been carved out as an interface ITypeCacheProvider > and the TypeSystem code has been refactored to use an instance of > ITypeCacheProvider injected through Guice. An instance of ITypeCacheProvider > is created by looking up the implementation class specified in > atlas-application.properties (property - atlas.typesystem.cache.provider). > The default implementation is a local cache implemented by > DefaultTypeCacheProvider. > > > Diffs > ----- > > .gitignore f5899e2069d29d71378cabe51900c120db19eef9 > common/src/main/java/org/apache/atlas/AtlasRuntimeException.java > PRE-CREATION > distro/src/conf/atlas-application.properties > 68a002115bdda99fba4dcace5ab97399c59289d9 > typesystem/src/main/java/org/apache/atlas/typesystem/types/TypeSystem.java > 7763ebc39593d2ba747bd53e861f1d35657c8ea1 > > typesystem/src/main/java/org/apache/atlas/typesystem/types/cache/DefaultTypeCacheProvider.java > PRE-CREATION > > typesystem/src/main/java/org/apache/atlas/typesystem/types/cache/ITypeCacheProvider.java > PRE-CREATION > > typesystem/src/test/java/org/apache/atlas/typesystem/types/cache/DefaultTypeCacheProviderTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/46330/diff/ > > > Testing > ------- > > Unit tests for the local cache DefaultTypeCacheProvider have been authored > and all other affected unit tests of TypeSystem interface are modified to > function in the presence of a type cache provider. Ran all tests of Atlas. > > > Thanks, > > venkata madugundu > >
