You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Hemanth Yamijala <yh...@gmail.com> on 2016/05/04 09:34:54 UTC
Re: Review Request 46650: ATLAS-585 NotificationHookConsumer creates
new AtlasClient for every message
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46650/#review131623
-----------------------------------------------------------
client/src/main/java/org/apache/atlas/AtlasAdminClient.java (line 63)
<https://reviews.apache.org/r/46650/#comment195648>
Not related to this patch, but since we are touching it, should we just get currentUGI and shortUserName here, as we are doing in quick start?
client/src/main/java/org/apache/atlas/AtlasClient.java
<https://reviews.apache.org/r/46650/#comment195642>
Can we retain the javadocs?
client/src/main/java/org/apache/atlas/AtlasClient.java (line 559)
<https://reviews.apache.org/r/46650/#comment195643>
Does this need to be public? Just trying to minimize exposure of this class to others.
webapp/src/main/java/org/apache/atlas/LocalAtlasClient.java (line 45)
<https://reviews.apache.org/r/46650/#comment195645>
Do we lose audit log information with this change for updates from NotificationHookConsumer? If yes, should we log these requests directly using the same logger in that case?
webapp/src/main/java/org/apache/atlas/LocalAtlasClient.java (line 122)
<https://reviews.apache.org/r/46650/#comment195656>
EntityResource is marked as a Singleton resource. While this doesn't prevent creation of multiple instances, it might just be confusing (for e.g. if someone makes changes to EntityResource on the basis of this). I don't feel strongly about it, but if we can do something (either remove the annotation, or get a single instance here) it might be nice to do.
webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java (line 101)
<https://reviews.apache.org/r/46650/#comment195652>
There appears to be a guava utility to do this: http://stackoverflow.com/a/9748680 - easier to use that?
webapp/src/main/java/org/apache/atlas/web/resources/TypesResource.java (line 85)
<https://reviews.apache.org/r/46650/#comment195653>
Since we are not overriding this in LocalAtlasClient, do we need this change?
webapp/src/main/java/org/apache/atlas/web/util/Servlets.java (line 86)
<https://reviews.apache.org/r/46650/#comment195654>
For my understanding, will any of our current clients be sending this query parameter?
- Hemanth Yamijala
On April 25, 2016, 6:15 p.m., Shwetha GS wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46650/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 6:15 p.m.)
>
>
> Review request for atlas.
>
>
> Bugs: ATLAS-585
> https://issues.apache.org/jira/browse/ATLAS-585
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Added local atlas client that calls the resources directly. So, it bypasses the filters
>
>
> Diffs
> -----
>
> addons/falcon-bridge/pom.xml 9b07c9f
> addons/hive-bridge/pom.xml e125f18
> addons/hive-bridge/src/main/java/org/apache/atlas/hive/bridge/HiveMetaStoreBridge.java 104c0c5
> addons/hive-bridge/src/test/java/org/apache/atlas/hive/hook/HiveHookIT.java b0d4c5c
> addons/sqoop-bridge/pom.xml 4b5dbb1
> addons/storm-bridge/pom.xml d8d98f5
> client/src/main/java/org/apache/atlas/AtlasAdminClient.java 473f72a
> client/src/main/java/org/apache/atlas/AtlasClient.java be34802
> client/src/main/java/org/apache/atlas/AtlasServiceException.java 6f68a71
> client/src/main/java/org/apache/atlas/security/SecureClientUtils.java d3b474a
> client/src/test/java/org/apache/atlas/AtlasClientTest.java 8911bf5
> distro/src/conf/atlas-log4j.xml 1ac4082
> notification/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 1f2df3e
> notification/src/test/java/org/apache/atlas/notification/NotificationHookConsumerTest.java 177de6d
> pom.xml 729b178
> repository/src/main/java/org/apache/atlas/repository/graph/GraphBackedSearchIndexer.java d83c08c
> webapp/pom.xml 1df4112
> webapp/src/main/java/org/apache/atlas/LocalAtlasClient.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/examples/QuickStart.java 79b8124
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java 01b1cd3
> webapp/src/main/java/org/apache/atlas/web/listeners/GuiceServletConfig.java 1eca174
> webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 6068007
> webapp/src/main/java/org/apache/atlas/web/resources/EntityResource.java b14aa80
> webapp/src/main/java/org/apache/atlas/web/resources/TypesResource.java 74d89ce
> webapp/src/main/java/org/apache/atlas/web/util/Servlets.java 480a232
> webapp/src/test/java/org/apache/atlas/LocalAtlasClientTest.java PRE-CREATION
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerIT.java 03a0d3f
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerTest.java PRE-CREATION
> webapp/src/test/java/org/apache/atlas/web/filters/AtlasAuthenticationKerberosFilterIT.java 9e1e08f
> webapp/src/test/java/org/apache/atlas/web/filters/AtlasAuthenticationKerberosFilterTest.java PRE-CREATION
> webapp/src/test/java/org/apache/atlas/web/filters/AtlasAuthenticationSimpleFilterIT.java ca53096
> webapp/src/test/java/org/apache/atlas/web/filters/AtlasAuthenticationSimpleFilterTest.java PRE-CREATION
> webapp/src/test/java/org/apache/atlas/web/resources/BaseResourceIT.java 54d8d92
> webapp/src/test/java/org/apache/atlas/web/resources/EntityJerseyResourceIT.java 720ce79
> webapp/src/test/java/org/apache/atlas/web/security/BaseSSLAndKerberosTest.java d497230
> webapp/src/test/java/org/apache/atlas/web/security/BaseSecurityTest.java 270a20d
> webapp/src/test/java/org/apache/atlas/web/security/SSLTest.java 521c037
>
> Diff: https://reviews.apache.org/r/46650/diff/
>
>
> Testing
> -------
>
> UTs and end to end test
>
>
> Thanks,
>
> Shwetha GS
>
>
Re: Review Request 46650: ATLAS-585 NotificationHookConsumer creates
new AtlasClient for every message
Posted by Shwetha GS <ss...@hortonworks.com>.
> On May 4, 2016, 7:34 a.m., Hemanth Yamijala wrote:
> > client/src/main/java/org/apache/atlas/AtlasAdminClient.java, line 63
> > <https://reviews.apache.org/r/46650/diff/1/?file=1360346#file1360346line63>
> >
> > Not related to this patch, but since we are touching it, should we just get currentUGI and shortUserName here, as we are doing in quick start?
Will add in AtlasClient itself
> On May 4, 2016, 7:34 a.m., Hemanth Yamijala wrote:
> > webapp/src/main/java/org/apache/atlas/LocalAtlasClient.java, line 122
> > <https://reviews.apache.org/r/46650/diff/1/?file=1360357#file1360357line122>
> >
> > EntityResource is marked as a Singleton resource. While this doesn't prevent creation of multiple instances, it might just be confusing (for e.g. if someone makes changes to EntityResource on the basis of this). I don't feel strongly about it, but if we can do something (either remove the annotation, or get a single instance here) it might be nice to do.
I wanted to override HttpServletRequest, but something didn't work at that time. I will try to clean it up
> On May 4, 2016, 7:34 a.m., Hemanth Yamijala wrote:
> > webapp/src/main/java/org/apache/atlas/web/util/Servlets.java, line 86
> > <https://reviews.apache.org/r/46650/diff/1/?file=1360365#file1360365line86>
> >
> > For my understanding, will any of our current clients be sending this query parameter?
AtlasClient uses DelegationTokenAuthenticatedURL which always sets doAs param. If there is proxy, there will be both doAs and user.name params. Without proxy, there will be just doAs param. So, everyone who uses AtlasClient will set doAs param
- Shwetha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46650/#review131623
-----------------------------------------------------------
On April 25, 2016, 6:15 p.m., Shwetha GS wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46650/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 6:15 p.m.)
>
>
> Review request for atlas.
>
>
> Bugs: ATLAS-585
> https://issues.apache.org/jira/browse/ATLAS-585
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Added local atlas client that calls the resources directly. So, it bypasses the filters
>
>
> Diffs
> -----
>
> addons/falcon-bridge/pom.xml 9b07c9f
> addons/hive-bridge/pom.xml e125f18
> addons/hive-bridge/src/main/java/org/apache/atlas/hive/bridge/HiveMetaStoreBridge.java 104c0c5
> addons/hive-bridge/src/test/java/org/apache/atlas/hive/hook/HiveHookIT.java b0d4c5c
> addons/sqoop-bridge/pom.xml 4b5dbb1
> addons/storm-bridge/pom.xml d8d98f5
> client/src/main/java/org/apache/atlas/AtlasAdminClient.java 473f72a
> client/src/main/java/org/apache/atlas/AtlasClient.java be34802
> client/src/main/java/org/apache/atlas/AtlasServiceException.java 6f68a71
> client/src/main/java/org/apache/atlas/security/SecureClientUtils.java d3b474a
> client/src/test/java/org/apache/atlas/AtlasClientTest.java 8911bf5
> distro/src/conf/atlas-log4j.xml 1ac4082
> notification/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 1f2df3e
> notification/src/test/java/org/apache/atlas/notification/NotificationHookConsumerTest.java 177de6d
> pom.xml 729b178
> repository/src/main/java/org/apache/atlas/repository/graph/GraphBackedSearchIndexer.java d83c08c
> webapp/pom.xml 1df4112
> webapp/src/main/java/org/apache/atlas/LocalAtlasClient.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/examples/QuickStart.java 79b8124
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java 01b1cd3
> webapp/src/main/java/org/apache/atlas/web/listeners/GuiceServletConfig.java 1eca174
> webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 6068007
> webapp/src/main/java/org/apache/atlas/web/resources/EntityResource.java b14aa80
> webapp/src/main/java/org/apache/atlas/web/resources/TypesResource.java 74d89ce
> webapp/src/main/java/org/apache/atlas/web/util/Servlets.java 480a232
> webapp/src/test/java/org/apache/atlas/LocalAtlasClientTest.java PRE-CREATION
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerIT.java 03a0d3f
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerTest.java PRE-CREATION
> webapp/src/test/java/org/apache/atlas/web/filters/AtlasAuthenticationKerberosFilterIT.java 9e1e08f
> webapp/src/test/java/org/apache/atlas/web/filters/AtlasAuthenticationKerberosFilterTest.java PRE-CREATION
> webapp/src/test/java/org/apache/atlas/web/filters/AtlasAuthenticationSimpleFilterIT.java ca53096
> webapp/src/test/java/org/apache/atlas/web/filters/AtlasAuthenticationSimpleFilterTest.java PRE-CREATION
> webapp/src/test/java/org/apache/atlas/web/resources/BaseResourceIT.java 54d8d92
> webapp/src/test/java/org/apache/atlas/web/resources/EntityJerseyResourceIT.java 720ce79
> webapp/src/test/java/org/apache/atlas/web/security/BaseSSLAndKerberosTest.java d497230
> webapp/src/test/java/org/apache/atlas/web/security/BaseSecurityTest.java 270a20d
> webapp/src/test/java/org/apache/atlas/web/security/SSLTest.java 521c037
>
> Diff: https://reviews.apache.org/r/46650/diff/
>
>
> Testing
> -------
>
> UTs and end to end test
>
>
> Thanks,
>
> Shwetha GS
>
>
Re: Review Request 46650: ATLAS-585 NotificationHookConsumer creates
new AtlasClient for every message
Posted by Hemanth Yamijala <yh...@gmail.com>.
> On May 4, 2016, 7:34 a.m., Hemanth Yamijala wrote:
> > client/src/main/java/org/apache/atlas/AtlasClient.java, line 130
> > <https://reviews.apache.org/r/46650/diff/1/?file=1360347#file1360347line130>
> >
> > Can we retain the javadocs?
There are still some deleted ones. This method, for e.g. public AtlasClient(UserGroupInformation ugi, String doAsUser, String... baseUrls). Also, multiple baseUrls indicate the active one will be detected and picked up. This was how the description was before.
- Hemanth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46650/#review131623
-----------------------------------------------------------
On May 5, 2016, 10:32 a.m., Shwetha GS wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46650/
> -----------------------------------------------------------
>
> (Updated May 5, 2016, 10:32 a.m.)
>
>
> Review request for atlas.
>
>
> Bugs: ATLAS-585
> https://issues.apache.org/jira/browse/ATLAS-585
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Added local atlas client that calls the resources directly. So, it bypasses the filters
>
>
> Diffs
> -----
>
> .gitignore ff7c32e
> addons/falcon-bridge/pom.xml 9b07c9f
> addons/hive-bridge/pom.xml e125f18
> addons/hive-bridge/src/main/java/org/apache/atlas/hive/bridge/HiveMetaStoreBridge.java 104c0c5
> addons/hive-bridge/src/test/java/org/apache/atlas/hive/hook/HiveHookIT.java b0d4c5c
> addons/sqoop-bridge/pom.xml 4b5dbb1
> addons/storm-bridge/pom.xml d8d98f5
> client/src/main/java/org/apache/atlas/AtlasAdminClient.java 473f72a
> client/src/main/java/org/apache/atlas/AtlasClient.java be34802
> client/src/main/java/org/apache/atlas/AtlasServiceException.java 6f68a71
> client/src/main/java/org/apache/atlas/security/SecureClientUtils.java d3b474a
> client/src/test/java/org/apache/atlas/AtlasClientTest.java 8911bf5
> distro/src/conf/atlas-log4j.xml 1ac4082
> notification/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java 1f2df3e
> notification/src/test/java/org/apache/atlas/notification/NotificationHookConsumerTest.java 177de6d
> pom.xml c14e0b3
> repository/src/main/java/org/apache/atlas/repository/graph/GraphBackedSearchIndexer.java d83c08c
> webapp/pom.xml 393863c
> webapp/src/main/java/org/apache/atlas/LocalAtlasClient.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/LocalServletRequest.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/examples/QuickStart.java 79b8124
> webapp/src/main/java/org/apache/atlas/notification/NotificationHookConsumer.java PRE-CREATION
> webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java 01b1cd3
> webapp/src/main/java/org/apache/atlas/web/filters/AuditFilter.java 9d60e1a
> webapp/src/main/java/org/apache/atlas/web/listeners/GuiceServletConfig.java 1eca174
> webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 6068007
> webapp/src/main/java/org/apache/atlas/web/resources/EntityResource.java b14aa80
> webapp/src/main/java/org/apache/atlas/web/util/Servlets.java 480a232
> webapp/src/test/java/org/apache/atlas/LocalAtlasClientTest.java PRE-CREATION
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerIT.java 03a0d3f
> webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerTest.java PRE-CREATION
> webapp/src/test/java/org/apache/atlas/web/filters/AtlasAuthenticationKerberosFilterIT.java 9e1e08f
> webapp/src/test/java/org/apache/atlas/web/filters/AtlasAuthenticationKerberosFilterTest.java PRE-CREATION
> webapp/src/test/java/org/apache/atlas/web/filters/AtlasAuthenticationSimpleFilterIT.java ca53096
> webapp/src/test/java/org/apache/atlas/web/filters/AtlasAuthenticationSimpleFilterTest.java PRE-CREATION
> webapp/src/test/java/org/apache/atlas/web/resources/BaseResourceIT.java 54d8d92
> webapp/src/test/java/org/apache/atlas/web/resources/EntityJerseyResourceIT.java 720ce79
> webapp/src/test/java/org/apache/atlas/web/security/BaseSSLAndKerberosTest.java d497230
> webapp/src/test/java/org/apache/atlas/web/security/BaseSecurityTest.java 270a20d
> webapp/src/test/java/org/apache/atlas/web/security/SSLTest.java 521c037
>
> Diff: https://reviews.apache.org/r/46650/diff/
>
>
> Testing
> -------
>
> UTs and end to end test
>
>
> Thanks,
>
> Shwetha GS
>
>