[
https://issues.apache.org/jira/browse/SENTRY-1329?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15334501#comment-15334501
]
Colin Patrick McCabe commented on SENTRY-1329:
----------------------------------------------
Thanks, [~sravya]. Looks great.
{code}
57 private RawStore rs;
58 private HiveConf hiveConf;
{code}
Can we make these final?
{code}
65 private synchronized void init(HiveConf conf) {
66 try {
67 this.rs = RawStoreProxy.getProxy(conf, conf,
conf.getVar(HiveConf.ConfVars.METASTORE_RAW_STORE_IMPL), 999999);
68 } catch (MetaException var3) {
69 LOGGER.error("Unable to connect to raw store, notifications will
not be tracked", var3);
70 this.rs = null;
71 }
{code}
This seems like it should be a fatal error for us, right? It would result in
Sentry not working. Similar with a lot of the errors in {{onCreateTable}},
etc. which seem to be ignored here. We should throw an exception in these
cases.
Shouldn't we have log4j {{LOG.trace}} statements (perhaps protected by "if"
blocks) to track calls into {{SentryMetastorePostEventListener}}?
{code}
352 try {
353 Thread.sleep(60000L);
354 } catch (InterruptedException var2) {
355 LOGGER.info("Cleaner thread sleep interupted", var2);
356 }
{code}
Hmm. If the thread was interrupted, the appropriate thing to do is to
termiante the thread, right? Swallowing {{InterruptedException}} is not a good
idea.
{code}
} catch (InterruptedException ex) {
Thread.currentThread().interrupt(); // Here!
throw new RuntimeException(ex);
}
{code}
See http://www.yegor256.com/2015/10/20/interrupted-exception.html
{code}
11 /**
12 * Created by sravya on 6/14/16.
13 */
{code}
Apache frowns on author tags, right?
It looks like this includes some of the code from SENTRY-1324. Is the plan to
get that JIRA into the branch first?
> Add additional information in the NotificationLog entry
> -------------------------------------------------------
>
> Key: SENTRY-1329
> URL: https://issues.apache.org/jira/browse/SENTRY-1329
> Project: Sentry
> Issue Type: Sub-task
> Components: Hdfs Plugin
> Reporter: Sravya Tirukkovalur
> Assignee: Sravya Tirukkovalur
> Fix For: sentry-ha-redesign
>
> Attachments: SENTRY-1329.0.patch
>
>
> After some preliminary testing of HMS NotificationLog in Sentry
> (SENTRY-1324), we see that NotificationLog does not capture some information
> today. See this [comment|
> https://issues.apache.org/jira/browse/SENTRY-1324?focusedCommentId=15330859&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15330859]
> for more information.
> So with respect to capturing this information, the minimally invasive
> approach is to just implement a custom MessageFactory
> (hcatalog.message.factory.impl.json), which takes care of the serialization
> and deseriazation of the message. We can just add additional information
> without causing disruption to other clients.
> As I was implementing this, I encountered the problem that there is a small
> bug(in Hive trunk) which makes the MessageFactory not truly pluggable
> (HIVE-14011 - Attached a fix). But it would be a while before Hive can make a
> release with this fix and Sentry can move to this fixed version.
> So in the interim, we can implement the Listener in Sentry and use custom
> MessageFactory as well. I have done some testing on my side to make sure it
> does not break other clients.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
