[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15734287#comment-15734287
]
Vadim Spector commented on SENTRY-1508:
---
Latest review board link: https://reviews.apache.org/r/53125/diff/14/
Seems like I've nailed down the cause of flaky HDFS integration tests. JUnit
creates multiple instances of MetastorePlugin (via multiple instances of Hive
metastore) and runs different tests on different instances. Each
MetastorePlugin starts its own housekeeping thread to synchronize HMS paths
with Sentry daemon. Since there is no proper shutdown API for MetastorePlugin
(design oversight), there is no proper way to shutdown housekeeping thread, so
multiple threads "synchronize" state with Sentry daemon, messing it up pretty
badly.
Added test hookup logic, shutting down previous housekeeping thread (if any
found) on each creation of new MetastorePlugin instance. This logic is no-op
in deployment scenarion, where only a single instance of MetastorePlugin is
created.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch, SENTRY-1508.005.patch,
> SENTRY-1508.006.patch, SENTRY-1508.007.patch, SENTRY-1508.008.patch,
> SENTRY-1508.009.patch, SENTRY-1508.010.patch, SENTRY-1508.011.patch,
> SENTRY-1508.012.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock()
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15734237#comment-15734237
]
Hadoop QA commented on SENTRY-1508:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12842478/SENTRY-1508.012.patch
against master.
{color:green}Overall:{color} +1 all checks pass
{color:green}SUCCESS:{color} all tests passed
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2196/console
This message is automatically generated.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch, SENTRY-1508.005.patch,
> SENTRY-1508.006.patch, SENTRY-1508.007.patch, SENTRY-1508.008.patch,
> SENTRY-1508.009.patch, SENTRY-1508.010.patch, SENTRY-1508.011.patch,
> SENTRY-1508.012.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15733845#comment-15733845
]
Hadoop QA commented on SENTRY-1508:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12842438/SENTRY-1508.011.patch
against master.
{color:red}Overall:{color} -1 due to 3 errors
{color:red}ERROR:{color} mvn test exited 1
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationAdvanced
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationAdvanced
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2194/console
This message is automatically generated.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch, SENTRY-1508.005.patch,
> SENTRY-1508.006.patch, SENTRY-1508.007.patch, SENTRY-1508.008.patch,
> SENTRY-1508.009.patch, SENTRY-1508.010.patch, SENTRY-1508.011.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15733377#comment-15733377
]
Hadoop QA commented on SENTRY-1508:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12842389/SENTRY-1508.010.patch
against master.
{color:red}Overall:{color} -1 due to 2 errors
{color:red}ERROR:{color} mvn test exited 1
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hive.TestPerDatabasePolicyFile
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2193/console
This message is automatically generated.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch, SENTRY-1508.005.patch,
> SENTRY-1508.006.patch, SENTRY-1508.007.patch, SENTRY-1508.008.patch,
> SENTRY-1508.009.patch, SENTRY-1508.010.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15710119#comment-15710119
]
Hadoop QA commented on SENTRY-1508:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12841128/SENTRY-1508.008.patch
against master.
{color:red}Overall:{color} -1 due to 2 errors
{color:red}ERROR:{color} mvn test exited 1
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.dbprovider.TestDbPrivilegesAtFunctionScope
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2174/console
This message is automatically generated.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch, SENTRY-1508.005.patch,
> SENTRY-1508.006.patch, SENTRY-1508.007.patch, SENTRY-1508.008.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15710027#comment-15710027
]
Hadoop QA commented on SENTRY-1508:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12841124/SENTRY-1508.007.patch
against master.
{color:red}Overall:{color} -1 due to 5 errors
{color:red}ERROR:{color} mvn test exited 1
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationEnd2End
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationEnd2End
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationEnd2End
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationEnd2End
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2173/console
This message is automatically generated.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch, SENTRY-1508.005.patch,
> SENTRY-1508.006.patch, SENTRY-1508.007.patch, SENTRY-1508.008.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15709745#comment-15709745
]
Vadim Spector commented on SENTRY-1508:
---
Latest code review:
https://reviews.apache.org/r/53125/diff/10/
Fixed a subtle problem having to do with initializing final variable. For some
reason, not picked up by command-line build system, might be some javac flags
...
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch, SENTRY-1508.005.patch,
> SENTRY-1508.006.patch, SENTRY-1508.007.patch, SENTRY-1508.008.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the broken state.
> b) however,
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15709681#comment-15709681
]
Vadim Spector commented on SENTRY-1508:
---
Latest code review: https://reviews.apache.org/r/53125/diff/9/
Nitpicking: polished top javadoc section describing MetastorePlugin
implementation
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch, SENTRY-1508.005.patch,
> SENTRY-1508.006.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the broken state.
> b) however, if the reviewers decide that support of asynchronous
> initialization is not necessary, it may be preferable to drop such
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15709424#comment-15709424
]
Vadim Spector commented on SENTRY-1508:
---
Latest code review link: https://reviews.apache.org/r/53125/diff/8/
Changes:
1. Removed all "volatile" definitions. Some are replaced with "final" via using
temp. variables in the constructor. Others are consistently guarded by
"notificationLock".
2. In one place, replaced try-catch-finally with try-with-resource (programming
style).
3. Misc. javadoc enhancements.
4. No bug fixes.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch, SENTRY-1508.005.patch,
> SENTRY-1508.006.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15707587#comment-15707587
]
Hadoop QA commented on SENTRY-1508:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12840978/SENTRY-1508.006.patch
against master.
{color:red}Overall:{color} -1 due to 5 errors
{color:red}ERROR:{color} mvn test exited 1
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationEnd2End
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationEnd2End
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationEnd2End
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationEnd2End
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2172/console
This message is automatically generated.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch, SENTRY-1508.005.patch,
> SENTRY-1508.006.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15688137#comment-15688137
]
Hadoop QA commented on SENTRY-1508:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12840107/SENTRY-1508.005.patch
against master.
{color:red}Overall:{color} -1 due to 2 errors
{color:red}ERROR:{color} mvn test exited 1
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationAdvanced
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2168/console
This message is automatically generated.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch, SENTRY-1508.005.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15687844#comment-15687844
]
Vadim Spector commented on SENTRY-1508:
---
Latest code review link: https://reviews.apache.org/r/53125/diff/7/
1. Added more javadoc per multiple requests.
2. More logical choice of method names (due to removed legacy support of
asynchronous initialization).
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch, SENTRY-1508.005.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the broken state.
> b) however, if the reviewers decide that support of asynchronous
> initialization is not
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15684374#comment-15684374
]
Vadim Spector commented on SENTRY-1508:
---
The latest code review: https://reviews.apache.org/r/53125/diff/6/
Only javadoc changes, per [~akolb]'s request.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the broken state.
> b) however, if the reviewers decide that support of asynchronous
> initialization is not necessary, it may be preferable to drop such support in
> the same JIRA. In this case, second patch can be generated to only
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15646073#comment-15646073
]
Hadoop QA commented on SENTRY-1508:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12837865/SENTRY-1508.004.patch
against master.
{color:red}Overall:{color} -1 due to 2 errors
{color:red}ERROR:{color} mvn test exited 1
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.dbprovider.TestDbPrivilegesAtFunctionScope
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2140/console
This message is automatically generated.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed,
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15645850#comment-15645850
]
Vadim Spector commented on SENTRY-1508:
---
Uploaded patch #4, with proper Javadoc formatting
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch, SENTRY-1508.004.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the broken state.
> b) however, if the reviewers decide that support of asynchronous
> initialization is not necessary, it may be preferable to drop such support in
> the same JIRA. In this case, second patch can be generated to only support
> synchronous initialization. This would lead to a much
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15635210#comment-15635210
]
Hadoop QA commented on SENTRY-1508:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12837031/SENTRY-1508.003.patch
against master.
{color:green}Overall:{color} +1 all checks pass
{color:green}SUCCESS:{color} all tests passed
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2127/console
This message is automatically generated.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the broken
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15635062#comment-15635062
]
Vadim Spector commented on SENTRY-1508:
---
Latest review board link, with all feedback integrated:
https://reviews.apache.org/r/53125/diff/5/
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch,
> SENTRY-1508.003.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the broken state.
> b) however, if the reviewers decide that support of asynchronous
> initialization is not necessary, it may be preferable to drop such support in
> the same JIRA. In this case, second patch can be generated to only support
> synchronous initialization. This
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15627047#comment-15627047
]
Alexander Kolbasov commented on SENTRY-1508:
[~vspec...@gmail.com] Can you also add reviewboard link?
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the broken state.
> b) however, if the reviewers decide that support of asynchronous
> initialization is not necessary, it may be preferable to drop such support in
> the same JIRA. In this case, second patch can be generated to only support
> synchronous initialization. This would lead to a much simpler and robust
> implementation.
--
This message was
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15626864#comment-15626864
]
Hadoop QA commented on SENTRY-1508:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12836446/SENTRY-1508.002.patch
against master.
{color:red}Overall:{color} -1 due to 3 errors
{color:red}ERROR:{color} mvn test exited 1
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.dbprovider.TestDbPrivilegesAtFunctionScope
{color:red}ERROR:{color} Failed:
org.apache.sentry.provider.db.service.thrift.TestSentryServerWithoutKerberos
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2119/console
This message is automatically generated.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
>
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15626793#comment-15626793
]
Hadoop QA commented on SENTRY-1508:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12836441/SENTRY-1508.002.patch
against master.
{color:red}Overall:{color} -1 due to 3 errors
{color:red}ERROR:{color} mvn test exited 1
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.dbprovider.TestDbPrivilegesAtFunctionScope
{color:red}ERROR:{color} Failed:
org.apache.sentry.provider.db.generic.service.persistent.TestPrivilegeOperatePersistence
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2118/console
This message is automatically generated.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15626566#comment-15626566
]
Hadoop QA commented on SENTRY-1508:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12836418/SENTRY-1508.002.patch
against master.
{color:red}Overall:{color} -1 due to 4 errors
{color:red}ERROR:{color} mvn test exited 1
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationAdvanced
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationAdvanced
{color:red}ERROR:{color} Failed:
org.apache.sentry.tests.e2e.hdfs.TestHDFSIntegrationAdvanced
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2117/console
This message is automatically generated.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15626294#comment-15626294
]
Vadim Spector commented on SENTRY-1508:
---
Patch is available, new code review: https://reviews.apache.org/r/53125/diff/3/
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the broken state.
> b) however, if the reviewers decide that support of asynchronous
> initialization is not necessary, it may be preferable to drop such support in
> the same JIRA. In this case, second patch can be generated to only support
> synchronous initialization. This would lead to a much simpler and robust
>
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15626281#comment-15626281
]
Vadim Spector commented on SENTRY-1508:
---
Per discussion with Sravya, all public callback APIs of MetastorePlugin -
addPath(), removePath(), renameAuthzObject() - should throw unchecked exception
upon catching SentryMalformedPathException, when the path passed to the method
is malformed (or null). It would clearly communicate to Hive code that Sentry
notification is broken. This is a minor change, patch will be available shortly.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch, SENTRY-1508.002.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15617027#comment-15617027
]
Hadoop QA commented on SENTRY-1508:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12835907/SENTRY-1508.001.patch
against master.
{color:green}Overall:{color} +1 all checks pass
{color:green}SUCCESS:{color} all tests passed
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2115/console
This message is automatically generated.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the broken state.
> b) however, if the reviewers decide that
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15616853#comment-15616853
]
Vadim Spector commented on SENTRY-1508:
---
Updated code review: https://reviews.apache.org/r/53125/diff/2/
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the broken state.
> b) however, if the reviewers decide that support of asynchronous
> initialization is not necessary, it may be preferable to drop such support in
> the same JIRA. In this case, second patch can be generated to only support
> synchronous initialization. This would lead to a much simpler and robust
> implementation.
--
This message was
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15616818#comment-15616818
]
Vadim Spector commented on SENTRY-1508:
---
Uploading brand-new patch with MetastorePlugin only supporting synchronous
initialization mode only. Found and fixed additional concurrency issues that
might result in Sentry updates being sent out-of-order (i.e. their sequence
numbers being non-monotonous).
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the broken state.
> b) however, if the reviewers decide that support of asynchronous
> initialization is not necessary, it may be preferable to drop such support in
> the same JIRA. In this case, second patch
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15606524#comment-15606524
]
Vadim Spector commented on SENTRY-1508:
---
Per our discussion, the following approach has been adopted:
a) Drop support of asynchronous initialization of MetastorePlugin. It will
significantly simplify the code, eliminating handling of
"initialization-in-progress" case (in particular updateQueue and syncSent -
related logic)
b) MetastorePlugin should still be initialized, even when cache initialization
has failed. This is in consideration of the existing client code (Hive daemon)
that would crush otherwise.
c) Failed cache initialization should make MetastorePlugin totally unusable. It
makes no sense (and would be misleading) to keep it partially functional. Every
public callback API should immediately throw the same exception with the
message clearly indicating the cause of initialization failure.
d) SENTRY-840 will be naturally addressed by this patch, since we are removing
support for async initialization.
e) the existing patch can be ignored; new patch to be generated shortly
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15606481#comment-15606481
]
Sravya Tirukkovalur commented on SENTRY-1508:
-
Linking related jiras here: Sentry-1260 attempts to improve error message at
the time of intializer failure and Sentry-1270 improves the error message for
following cases when intialization has failed. SENTRY-840 proposes to get rid
of the async configurability.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the broken state.
> b) however, if the reviewers decide that support of asynchronous
> initialization is not necessary, it may be preferable to drop
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15606181#comment-15606181
]
Vadim Spector commented on SENTRY-1508:
---
I don't believe I've seen expected semantics documented anywhere, so I can only
say what the code attempts to implement.
The existing code's behavior does not distinguish between the two states. The
constructor tries to initialize the local cache of authorized paths
(authzPaths) and if it fails, authzPaths always stays null with no attempt to
re-initialize it. Lack of re-initialization attempt actually makes sense,
because when some invalid HMS entries break initialization, it must be fixed
first, so retrying would be futile. MetastorePlugin still gets constructed and
Hive code has no problems using it.
Once authzPaths fails to initialize (or is not yet initialized), it stays null.
The behavior of MetastorePlugin is as follows:
1) the synchronization thread (SyncTask) periodically makes sure that the
local cache is in-sync with the Sentry-side. It will short-circuit when
authzPaths == null. This fact is invisible to the client code, unless one
checks the logs. The result - increased odds of Sentry state getting
out-of-sync.
2) Each public callback API (addPath, removeAllPaths, removePath,
renameAuthzObject) does two things, in that order:
a) updates local cache, i.e. authzPaths, which, of course, fails if authzPaths
== null due to either initialization-in-progress, or failed initialization.
b) attempts to notify Sentry, which fails whenever (a) throws an exception,
which happens for the reasons explained in (a). Sentry notification, actually,
can fail as well, due to the comm error, and fail silently (only checking the
logs would reveal the failure) - which is another issue.
No direct way exists for the client to check whether MetastorePlugin has been
initialized successfully, apart from consistent failure of callback APIs.
The scope of the initial patch is to simplify detection and troubleshooting of
the failed cache initialization + plus proactively fix some found thread
synchronization issues affecting the normal flow. I believe, there should be no
way of using MetastorePlugin after failed initialization, and it's
straightforward to fix - but that that would certainly affect public APIs
behavior and may affect Hive code. If we remove support for async
initialization, then we address initialization failure by simply failing to
construct MetastorePlugin (which would also affect Hive code). It is up to us
to decide whether it is worth spending time on, or we should wait for HA where
we'll hopefully do things right.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException.
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15603751#comment-15603751
]
Alexander Kolbasov commented on SENTRY-1508:
What is the expected semantics of callers to various MetastorePlugin functions
when
* MetastorePlugin initialization isn't complete - should they wait or go ahead?
* MetastorePlugin initialization failed - should they also fail or try to
recover?
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original design supporting asynchronous
> synchronization. It means that MetastorePlugin is always constructed, even if
> it's in a broken state. It is up to the public callback APIs to properly
> inform the caller of the broken state.
> b) however, if the reviewers decide that support of asynchronous
> initialization is not necessary, it may be preferable to drop such support in
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15603332#comment-15603332
]
Vadim Spector commented on SENTRY-1508:
---
I edited the description to make it more clear what this JIRA is/ is not going
to address.
To re-iterate: this JIRA honors the original design, which is supporting
asynchronous initialization of MetastorePlugin; it just tries to fix the
implementation bugs, and those bugs should all be covered in the Description.
However, should we decide to drop support of asynchronous initialization
altogether (perhaps, as the result of code review and thinking of all the
implementation complexities?), I'd be strongly in favor of it. In this case,
I'd rather submit another patch within the same JIRA, so no, I do not propose
another JIRA.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java has several implementation issues that need to be fixed
> by this JIRA:
> a) The state of MetastorePlugin is not communicated cleanly. The constructor
> initializes successfully regardless of whether HMS cache initialization was
> successful or not. The initThread.run() can easily fail when, say,
> cacheInitializer.createInitialUpdate() fails due to misconfigured HMS, so it
> is quite possible. The state is communicated through 3 variables: boolean
> initComplete, Throwable initError, and UpdateableAuthzPaths authzPaths.
> initComplete is always set to true from finally {} block, so it really
> indicates the end of an _attempt_ to initialize the cache. It, thus, should
> only be used to detect whether the cache initialization is still in-progress,
> not whether initialization has been successful. To determine whether cache
> initialization was successful, initComplete should be used together with
> initError, which is set only when initialization fails. This is not how the
> code implements it in more than one place, which should be clear from
> analyzing a patch.
> b) There are several synchronization issues that may lead to the failure of
> synchronizing with the Sentry. They usually have to do with inconsistent use
> of monitor objects to guard access to thread-unsafe objects. E.g.
> authzPaths.updatePartial() uses a lock created just for the scope of a single
> call, which makes it useless for synchronization. A single read-write lock
> should be used instead, as there is one read and one write operation
> performed on authzPaths within MetastorePlugin.
> c) Failure to create authzPaths is not communicated clearly to a caller. When
> it is dereferenced from the public callback APIs, it results in
> NullPointerException. The suggested fix of this issue in SENTRY-1270 avoids
> NullPointerException by detecting the null authzPaths, printing error
> message, and returning. This effectively leads to consistent failure to
> update local cache without notifying the caller. The suggested solution would
> be not only to throw IllegalArgumentException to the caller instead, but to
> also specify why exactly authzPaths == null - due to initialization still
> being in progress or due to its failure.
> d) The housekeeping thread running SyncTask to synchronize with Sentry state
> fails when authzPaths == null. However, it keeps printing misleading "
> Metastore Plugin cache has not finished initialization." message even in the
> case of a permanent cache initialization failure. It needs to print the real
> cause of this condition, by analyzing authzPaths together with initComplete
> and initError values.
> e) getClient() may return null, in which case dereferencing it causes not so
> helpful NullPointerException. Instead, while getClient() may still print
> error message, it should also re-throw an original exception, which would
> then be much easier to debug.
> f) Each code fork deserves log message: e.g. addPath() retrurns immediately
> if PathsUpdate.parsePath() returns null - w/o printing any log.
> g) in SyncTask.run() - if notificationLock.tryLock() succeeds, yet the next
> line's expression "MetastorePlugin.this.authzPaths == null" is evaluated to
> false, run() exits w/o a chance to call notificationLock.unlock(). All the
> code following lock.tryLock() should be inside try-catch-finally, with
> finally's first line being lock.unlock(), as a general safe pattern.
> h) some additional misc synchronization issues may also be addressed by the
> patch.
> What's not in the scope:
> a) the initial patch honors the original
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15603240#comment-15603240
]
Hadoop QA commented on SENTRY-1508:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12834768/SENTRY-1508.001.patch
against master.
{color:green}Overall:{color} +1 all checks pass
{color:green}SUCCESS:{color} all tests passed
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2084/console
This message is automatically generated.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java constructor initializes successfully regardless of
> whether initThread.run() was successful. E.g. if
> cacheInitializer.createInitialUpdate() cal inside the constructor fails (in
> one case in the field it was due to an invalid table uri in HMS database,
> whose parsing produced ArrayIndexOutOfBoundsException), initComplete is still
> assigned to true and MetastorePlugin gets constructed. While initError is
> assigned to non-null value in case of failure, it is effectively ignored as
> will be described later.
> MetastorePlugin implements several callbacks to be called on the Hive side,
> to update Sentry state. It is critical functionality for enforcing access
> control. Its callbacks (addPath(), removeAllPaths(), etc) all end up calling
> notifySentryAndApplyLocal() which starts with the following code:
> if (initComplete) {
> processUpdate()
> }...
> which means that processUpdate() is called even when MetastorePlugin
> initialization has failed (i.e. initError != null). There is an option of
> asynchronous initialization, when MetastorePlugin is constructed and returned
> to the caller before initialization is complete, but the code makes no
> distinction between the two cases, as will be shown below.
> processUpdate() called regardless of whether MetastorePlugin a) has been
> initialized (sync mode, success), or b) still being initialized (async mode),
> or c) failed to initialized (either mode). It calls (unconditionally)
> applyLocal() and notifySentry(). aplyLocal() calls authzPaths.updatePartial()
> which immediately fails with NullPointerException if authzPaths == null which
> happens if initThread within the constructor fails.
> SENTRY-1270 avoids NullPointerException by execuitng this before
> dereferencing authzPaths inside applyLocal():
> if(authzPaths == null) {
> LOGGER.error(initializationFailureMsg);
> return;
> }
> However, this leads to hard-to-diagnose behavior, because a) local updates
> fail forever, and b) housekeeping thread running SyncTask to synchronize with
> Sentry-side state fails as well, printing endlessly misleading "
> Metastore Plugin cache has not finished initialization." message suggesting
> its temporary condition as opposed to a permanent failure. Failure to sync up
> with Sentry state can lead to very subtle, often intermittent, problems with
> acls. MetastorePlugin and its caller never get fully aware of the permanent
> nature of init failure.
> Suggestion:
> a) define 3 states in MetastorePlugin: initializing, initialized, init_failed.
> 2) communicate those states to the caller clearly, by _immediately_ throwing
> some kind of exception from public callback APIs in the cases preventing
> those callback from completing successfully:
> a) for state == initializing, throw some kind of
> InitializationInProgressException. This only applies for asynchronous
> initialization configuration. It is unknown whether it is actually deployed
> anywhere, but the code exists and there is no reason why not to support it.
> Async init makes sense as an optimization, as long as
> initialization-in-progress condition is clearly communicated.
>b) for state == init_failed throw InitializationFailedException of some
> sort. For usability, it should contain the cause - the original exception
> originating either from the constructor or from the initThread.run() started
> from the constructor (in sync or async modes alike).
> Although in the sync mode initialization failure could be detected easily by
> constructor failure, we may want to preserve the current design, in which
> constructor always succeeds, so the client code does not need to be aware of
> whether init mode is configured as sync vs async.
> Additional cleanup:
> 1. getClient() is always dereferenced, while it can actually return null
> (exception is swallowed and error is logged).
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15603218#comment-15603218
]
Alexander Kolbasov commented on SENTRY-1508:
Can you clarify what aspects are you fixing with the suggested patch and what
aspects you are leaving alone? Do you plan to open another JIRA for things that
are not addressed here?
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java constructor initializes successfully regardless of
> whether initThread.run() was successful. E.g. if
> cacheInitializer.createInitialUpdate() cal inside the constructor fails (in
> one case in the field it was due to an invalid table uri in HMS database,
> whose parsing produced ArrayIndexOutOfBoundsException), initComplete is still
> assigned to true and MetastorePlugin gets constructed. While initError is
> assigned to non-null value in case of failure, it is effectively ignored as
> will be described later.
> MetastorePlugin implements several callbacks to be called on the Hive side,
> to update Sentry state. It is critical functionality for enforcing access
> control. Its callbacks (addPath(), removeAllPaths(), etc) all end up calling
> notifySentryAndApplyLocal() which starts with the following code:
> if (initComplete) {
> processUpdate()
> }...
> which means that processUpdate() is called even when MetastorePlugin
> initialization has failed (i.e. initError != null). There is an option of
> asynchronous initialization, when MetastorePlugin is constructed and returned
> to the caller before initialization is complete, but the code makes no
> distinction between the two cases, as will be shown below.
> processUpdate() called regardless of whether MetastorePlugin a) has been
> initialized (sync mode, success), or b) still being initialized (async mode),
> or c) failed to initialized (either mode). It calls (unconditionally)
> applyLocal() and notifySentry(). aplyLocal() calls authzPaths.updatePartial()
> which immediately fails with NullPointerException if authzPaths == null which
> happens if initThread within the constructor fails.
> SENTRY-1270 avoids NullPointerException by execuitng this before
> dereferencing authzPaths inside applyLocal():
> if(authzPaths == null) {
> LOGGER.error(initializationFailureMsg);
> return;
> }
> However, this leads to hard-to-diagnose behavior, because a) local updates
> fail forever, and b) housekeeping thread running SyncTask to synchronize with
> Sentry-side state fails as well, printing endlessly misleading "
> Metastore Plugin cache has not finished initialization." message suggesting
> its temporary condition as opposed to a permanent failure. Failure to sync up
> with Sentry state can lead to very subtle, often intermittent, problems with
> acls. MetastorePlugin and its caller never get fully aware of the permanent
> nature of init failure.
> Suggestion:
> a) define 3 states in MetastorePlugin: initializing, initialized, init_failed.
> 2) communicate those states to the caller clearly, by _immediately_ throwing
> some kind of exception from public callback APIs in the cases preventing
> those callback from completing successfully:
> a) for state == initializing, throw some kind of
> InitializationInProgressException. This only applies for asynchronous
> initialization configuration. It is unknown whether it is actually deployed
> anywhere, but the code exists and there is no reason why not to support it.
> Async init makes sense as an optimization, as long as
> initialization-in-progress condition is clearly communicated.
>b) for state == init_failed throw InitializationFailedException of some
> sort. For usability, it should contain the cause - the original exception
> originating either from the constructor or from the initThread.run() started
> from the constructor (in sync or async modes alike).
> Although in the sync mode initialization failure could be detected easily by
> constructor failure, we may want to preserve the current design, in which
> constructor always succeeds, so the client code does not need to be aware of
> whether init mode is configured as sync vs async.
> Additional cleanup:
> 1. getClient() is always dereferenced, while it can actually return null
> (exception is swallowed and error is logged). Suggested fix: getClient() may
> still print error message, but it should also re-throw an original exception
> - it is much easier to debug than dealing with inevitable and
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15600543#comment-15600543
]
Vadim Spector commented on SENTRY-1508:
---
Code review: https://reviews.apache.org/r/53125/
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java constructor initializes successfully regardless of
> whether initThread.run() was successful. E.g. if
> cacheInitializer.createInitialUpdate() cal inside the constructor fails (in
> one case in the field it was due to an invalid table uri in HMS database,
> whose parsing produced ArrayIndexOutOfBoundsException), initComplete is still
> assigned to true and MetastorePlugin gets constructed. While initError is
> assigned to non-null value in case of failure, it is effectively ignored as
> will be described later.
> MetastorePlugin implements several callbacks to be called on the Hive side,
> to update Sentry state. It is critical functionality for enforcing access
> control. Its callbacks (addPath(), removeAllPaths(), etc) all end up calling
> notifySentryAndApplyLocal() which starts with the following code:
> if (initComplete) {
> processUpdate()
> }...
> which means that processUpdate() is called even when MetastorePlugin
> initialization has failed (i.e. initError != null). There is an option of
> asynchronous initialization, when MetastorePlugin is constructed and returned
> to the caller before initialization is complete, but the code makes no
> distinction between the two cases, as will be shown below.
> processUpdate() called regardless of whether MetastorePlugin a) has been
> initialized (sync mode, success), or b) still being initialized (async mode),
> or c) failed to initialized (either mode). It calls (unconditionally)
> applyLocal() and notifySentry(). aplyLocal() calls authzPaths.updatePartial()
> which immediately fails with NullPointerException if authzPaths == null which
> happens if initThread within the constructor fails.
> SENTRY-1270 avoids NullPointerException by execuitng this before
> dereferencing authzPaths inside applyLocal():
> if(authzPaths == null) {
> LOGGER.error(initializationFailureMsg);
> return;
> }
> However, this leads to hard-to-diagnose behavior, because a) local updates
> fail forever, and b) housekeeping thread running SyncTask to synchronize with
> Sentry-side state fails as well, printing endlessly misleading "
> Metastore Plugin cache has not finished initialization." message suggesting
> its temporary condition as opposed to a permanent failure. Failure to sync up
> with Sentry state can lead to very subtle, often intermittent, problems with
> acls. MetastorePlugin and its caller never get fully aware of the permanent
> nature of init failure.
> Suggestion:
> a) define 3 states in MetastorePlugin: initializing, initialized, init_failed.
> 2) communicate those states to the caller clearly, by _immediately_ throwing
> some kind of exception from public callback APIs in the cases preventing
> those callback from completing successfully:
> a) for state == initializing, throw some kind of
> InitializationInProgressException. This only applies for asynchronous
> initialization configuration. It is unknown whether it is actually deployed
> anywhere, but the code exists and there is no reason why not to support it.
> Async init makes sense as an optimization, as long as
> initialization-in-progress condition is clearly communicated.
>b) for state == init_failed throw InitializationFailedException of some
> sort. For usability, it should contain the cause - the original exception
> originating either from the constructor or from the initThread.run() started
> from the constructor (in sync or async modes alike).
> Although in the sync mode initialization failure could be detected easily by
> constructor failure, we may want to preserve the current design, in which
> constructor always succeeds, so the client code does not need to be aware of
> whether init mode is configured as sync vs async.
> Additional cleanup:
> 1. getClient() is always dereferenced, while it can actually return null
> (exception is swallowed and error is logged). Suggested fix: getClient() may
> still print error message, but it should also re-throw an original exception
> - it is much easier to debug than dealing with inevitable and uninformative
> NullPointerException up the caller stack. General rule - error messages are
> to help with diagnostic, not to replace throwing exception, which is the only
> way to
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15600288#comment-15600288
]
Alexander Kolbasov commented on SENTRY-1508:
[~vspec...@gmail.com]
Can you post a reviewboard link for your changes?
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java constructor initializes successfully regardless of
> whether initThread.run() was successful. E.g. if
> cacheInitializer.createInitialUpdate() cal inside the constructor fails (in
> one case in the field it was due to an invalid table uri in HMS database,
> whose parsing produced ArrayIndexOutOfBoundsException), initComplete is still
> assigned to true and MetastorePlugin gets constructed. While initError is
> assigned to non-null value in case of failure, it is effectively ignored as
> will be described later.
> MetastorePlugin implements several callbacks to be called on the Hive side,
> to update Sentry state. It is critical functionality for enforcing access
> control. Its callbacks (addPath(), removeAllPaths(), etc) all end up calling
> notifySentryAndApplyLocal() which starts with the following code:
> if (initComplete) {
> processUpdate()
> }...
> which means that processUpdate() is called even when MetastorePlugin
> initialization has failed (i.e. initError != null). There is an option of
> asynchronous initialization, when MetastorePlugin is constructed and returned
> to the caller before initialization is complete, but the code makes no
> distinction between the two cases, as will be shown below.
> processUpdate() called regardless of whether MetastorePlugin a) has been
> initialized (sync mode, success), or b) still being initialized (async mode),
> or c) failed to initialized (either mode). It calls (unconditionally)
> applyLocal() and notifySentry(). aplyLocal() calls authzPaths.updatePartial()
> which immediately fails with NullPointerException if authzPaths == null which
> happens if initThread within the constructor fails.
> SENTRY-1270 avoids NullPointerException by execuitng this before
> dereferencing authzPaths inside applyLocal():
> if(authzPaths == null) {
> LOGGER.error(initializationFailureMsg);
> return;
> }
> However, this leads to hard-to-diagnose behavior, because a) local updates
> fail forever, and b) housekeeping thread running SyncTask to synchronize with
> Sentry-side state fails as well, printing endlessly misleading "
> Metastore Plugin cache has not finished initialization." message suggesting
> its temporary condition as opposed to a permanent failure. Failure to sync up
> with Sentry state can lead to very subtle, often intermittent, problems with
> acls. MetastorePlugin and its caller never get fully aware of the permanent
> nature of init failure.
> Suggestion:
> a) define 3 states in MetastorePlugin: initializing, initialized, init_failed.
> 2) communicate those states to the caller clearly, by _immediately_ throwing
> some kind of exception from public callback APIs in the cases preventing
> those callback from completing successfully:
> a) for state == initializing, throw some kind of
> InitializationInProgressException. This only applies for asynchronous
> initialization configuration. It is unknown whether it is actually deployed
> anywhere, but the code exists and there is no reason why not to support it.
> Async init makes sense as an optimization, as long as
> initialization-in-progress condition is clearly communicated.
>b) for state == init_failed throw InitializationFailedException of some
> sort. For usability, it should contain the cause - the original exception
> originating either from the constructor or from the initThread.run() started
> from the constructor (in sync or async modes alike).
> Although in the sync mode initialization failure could be detected easily by
> constructor failure, we may want to preserve the current design, in which
> constructor always succeeds, so the client code does not need to be aware of
> whether init mode is configured as sync vs async.
> Additional cleanup:
> 1. getClient() is always dereferenced, while it can actually return null
> (exception is swallowed and error is logged). Suggested fix: getClient() may
> still print error message, but it should also re-throw an original exception
> - it is much easier to debug than dealing with inevitable and uninformative
> NullPointerException up the caller stack. General rule - error messages are
> to help with diagnostic, not to replace throwing
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15600233#comment-15600233
]
Alexander Kolbasov commented on SENTRY-1508:
I agree that the synchronization for the MetastorePlugin is quite broken at
various places.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java constructor initializes successfully regardless of
> whether initThread.run() was successful. E.g. if
> cacheInitializer.createInitialUpdate() cal inside the constructor fails (in
> one case in the field it was due to an invalid table uri in HMS database,
> whose parsing produced ArrayIndexOutOfBoundsException), initComplete is still
> assigned to true and MetastorePlugin gets constructed. While initError is
> assigned to non-null value in case of failure, it is effectively ignored as
> will be described later.
> MetastorePlugin implements several callbacks to be called on the Hive side,
> to update Sentry state. It is critical functionality for enforcing access
> control. Its callbacks (addPath(), removeAllPaths(), etc) all end up calling
> notifySentryAndApplyLocal() which starts with the following code:
> if (initComplete) {
> processUpdate()
> }...
> which means that processUpdate() is called even when MetastorePlugin
> initialization has failed (i.e. initError != null). There is an option of
> asynchronous initialization, when MetastorePlugin is constructed and returned
> to the caller before initialization is complete, but the code makes no
> distinction between the two cases, as will be shown below.
> processUpdate() called regardless of whether MetastorePlugin a) has been
> initialized (sync mode, success), or b) still being initialized (async mode),
> or c) failed to initialized (either mode). It calls (unconditionally)
> applyLocal() and notifySentry(). aplyLocal() calls authzPaths.updatePartial()
> which immediately fails with NullPointerException if authzPaths == null which
> happens if initThread within the constructor fails.
> SENTRY-1270 avoids NullPointerException by execuitng this before
> dereferencing authzPaths inside applyLocal():
> if(authzPaths == null) {
> LOGGER.error(initializationFailureMsg);
> return;
> }
> However, this leads to hard-to-diagnose behavior, because a) local updates
> fail forever, and b) housekeeping thread running SyncTask to synchronize with
> Sentry-side state fails as well, printing endlessly misleading "
> Metastore Plugin cache has not finished initialization." message suggesting
> its temporary condition as opposed to a permanent failure. Failure to sync up
> with Sentry state can lead to very subtle, often intermittent, problems with
> acls. MetastorePlugin and its caller never get fully aware of the permanent
> nature of init failure.
> Suggestion:
> a) define 3 states in MetastorePlugin: initializing, initialized, init_failed.
> 2) communicate those states to the caller clearly, by _immediately_ throwing
> some kind of exception from public callback APIs in the cases preventing
> those callback from completing successfully:
> a) for state == initializing, throw some kind of
> InitializationInProgressException. This only applies for asynchronous
> initialization configuration. It is unknown whether it is actually deployed
> anywhere, but the code exists and there is no reason why not to support it.
> Async init makes sense as an optimization, as long as
> initialization-in-progress condition is clearly communicated.
>b) for state == init_failed throw InitializationFailedException of some
> sort. For usability, it should contain the cause - the original exception
> originating either from the constructor or from the initThread.run() started
> from the constructor (in sync or async modes alike).
> Although in the sync mode initialization failure could be detected easily by
> constructor failure, we may want to preserve the current design, in which
> constructor always succeeds, so the client code does not need to be aware of
> whether init mode is configured as sync vs async.
> Additional cleanup:
> 1. getClient() is always dereferenced, while it can actually return null
> (exception is swallowed and error is logged). Suggested fix: getClient() may
> still print error message, but it should also re-throw an original exception
> - it is much easier to debug than dealing with inevitable and uninformative
> NullPointerException up the caller stack. General rule - error messages are
> to help with diagnostic, not to
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15600043#comment-15600043
]
Vadim Spector commented on SENTRY-1508:
---
I think it is a legitimate assumption that whenever there is a class taking
time to initialize, the client of such class may want it to happen
asynchronously, to keep the main thread alive. The question is - on who's side
it is implemented.
The same could be done in the client code by simply creating MetastorePlugin on
a separate thread and allowing callbacks only after creation is complete, at
which point complete initialization is guaranteed. This approach has many
advantages, one of them being a very simple state model - once you can get
reference to MetastorePlugin, it is fully functional.
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java constructor initializes successfully regardless of
> whether initThread.run() was successful. E.g. if
> cacheInitializer.createInitialUpdate() cal inside the constructor fails (in
> one case in the field it was due to an invalid table uri in HMS database,
> whose parsing produced ArrayIndexOutOfBoundsException), initComplete is still
> assigned to true and MetastorePlugin gets constructed. While initError is
> assigned to non-null value in case of failure, it is effectively ignored as
> will be described later.
> MetastorePlugin implements several callbacks to be called on the Hive side,
> to update Sentry state. It is critical functionality for enforcing access
> control. Its callbacks (addPath(), removeAllPaths(), etc) all end up calling
> notifySentryAndApplyLocal() which starts with the following code:
> if (initComplete) {
> processUpdate()
> }...
> which means that processUpdate() is called even when MetastorePlugin
> initialization has failed (i.e. initError != null). There is an option of
> asynchronous initialization, when MetastorePlugin is constructed and returned
> to the caller before initialization is complete, but the code makes no
> distinction between the two cases, as will be shown below.
> processUpdate() called regardless of whether MetastorePlugin a) has been
> initialized (sync mode, success), or b) still being initialized (async mode),
> or c) failed to initialized (either mode). It calls (unconditionally)
> applyLocal() and notifySentry(). aplyLocal() calls authzPaths.updatePartial()
> which immediately fails with NullPointerException if authzPaths == null which
> happens if initThread within the constructor fails.
> SENTRY-1270 avoids NullPointerException by execuitng this before
> dereferencing authzPaths inside applyLocal():
> if(authzPaths == null) {
> LOGGER.error(initializationFailureMsg);
> return;
> }
> However, this leads to hard-to-diagnose behavior, because a) local updates
> fail forever, and b) housekeeping thread running SyncTask to synchronize with
> Sentry-side state fails as well, printing endlessly misleading "
> Metastore Plugin cache has not finished initialization." message suggesting
> its temporary condition as opposed to a permanent failure. Failure to sync up
> with Sentry state can lead to very subtle, often intermittent, problems with
> acls. MetastorePlugin and its caller never get fully aware of the permanent
> nature of init failure.
> Suggestion:
> a) define 3 states in MetastorePlugin: initializing, initialized, init_failed.
> 2) communicate those states to the caller clearly, by _immediately_ throwing
> some kind of exception from public callback APIs in the cases preventing
> those callback from completing successfully:
> a) for state == initializing, throw some kind of
> InitializationInProgressException. This only applies for asynchronous
> initialization configuration. It is unknown whether it is actually deployed
> anywhere, but the code exists and there is no reason why not to support it.
> Async init makes sense as an optimization, as long as
> initialization-in-progress condition is clearly communicated.
>b) for state == init_failed throw InitializationFailedException of some
> sort. For usability, it should contain the cause - the original exception
> originating either from the constructor or from the initThread.run() started
> from the constructor (in sync or async modes alike).
> Although in the sync mode initialization failure could be detected easily by
> constructor failure, we may want to preserve the current design, in which
> constructor always succeeds, so the client code does not need to be aware of
[jira] [Commented] (SENTRY-1508) MetastorePlugin.java does not handle properly initialization failure
[
https://issues.apache.org/jira/browse/SENTRY-1508?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15598711#comment-15598711
]
Alexander Kolbasov commented on SENTRY-1508:
What is the point of doing async initialization in constructor?
> MetastorePlugin.java does not handle properly initialization failure
>
>
> Key: SENTRY-1508
> URL: https://issues.apache.org/jira/browse/SENTRY-1508
> Project: Sentry
> Issue Type: Bug
>Reporter: Vadim Spector
>Priority: Critical
> Attachments: SENTRY-1508.001.patch
>
>
> MetastorePlugin.java constructor initializes successfully regardless of
> whether initThread.run() was successful. E.g. if
> cacheInitializer.createInitialUpdate() cal inside the constructor fails (in
> one case in the field it was due to an invalid table uri in HMS database,
> whose parsing produced ArrayIndexOutOfBoundsException), initComplete is still
> assigned to true and MetastorePlugin gets constructed. While initError is
> assigned to non-null value in case of failure, it is effectively ignored as
> will be described later.
> MetastorePlugin implements several callbacks to be called on the Hive side,
> to update Sentry state. It is critical functionality for enforcing access
> control. Its callbacks (addPath(), removeAllPaths(), etc) all end up calling
> notifySentryAndApplyLocal() which starts with the following code:
> if (initComplete) {
> processUpdate()
> }...
> which means that processUpdate() is called even when MetastorePlugin
> initialization has failed (i.e. initError != null). There is an option of
> asynchronous initialization, when MetastorePlugin is constructed and returned
> to the caller before initialization is complete, but the code makes no
> distinction between the two cases, as will be shown below.
> processUpdate() called regardless of whether MetastorePlugin a) has been
> initialized (sync mode, success), or b) still being initialized (async mode),
> or c) failed to initialized (either mode). It calls (unconditionally)
> applyLocal() and notifySentry(). aplyLocal() calls authzPaths.updatePartial()
> which immediately fails with NullPointerException if authzPaths == null which
> happens if initThread within the constructor fails.
> SENTRY-1270 avoids NullPointerException by execuitng this before
> dereferencing authzPaths inside applyLocal():
> if(authzPaths == null) {
> LOGGER.error(initializationFailureMsg);
> return;
> }
> However, this leads to hard-to-diagnose behavior, because a) local updates
> fail forever, and b) housekeeping thread running SyncTask to synchronize with
> Sentry-side state fails as well, printing endlessly misleading "
> Metastore Plugin cache has not finished initialization." message suggesting
> its temporary condition as opposed to a permanent failure. Failure to sync up
> with Sentry state can lead to very subtle, often intermittent, problems with
> acls. MetastorePlugin and its caller never get fully aware of the permanent
> nature of init failure.
> Suggestion:
> a) define 3 states in MetastorePlugin: initializing, initialized, init_failed.
> 2) communicate those states to the caller clearly, by _immediately_ throwing
> some kind of exception from public callback APIs in the cases preventing
> those callback from completing successfully:
> a) for state == initializing, throw some kind of
> InitializationInProgressException. This only applies for asynchronous
> initialization configuration. It is unknown whether it is actually deployed
> anywhere, but the code exists and there is no reason why not to support it.
> Async init makes sense as an optimization, as long as
> initialization-in-progress condition is clearly communicated.
>b) for state == init_failed throw InitializationFailedException of some
> sort. For usability, it should contain the cause - the original exception
> originating either from the constructor or from the initThread.run() started
> from the constructor (in sync or async modes alike).
> Although in the sync mode initialization failure could be detected easily by
> constructor failure, we may want to preserve the current design, in which
> constructor always succeeds, so the client code does not need to be aware of
> whether init mode is configured as sync vs async.
> Additional cleanup:
> 1. getClient() is always dereferenced, while it can actually return null
> (exception is swallowed and error is logged). Suggested fix: getClient() may
> still print error message, but it should also re-throw an original exception
> - it is much easier to debug than dealing with inevitable and uninformative
> NullPointerException up the caller stack. General rule - error messages are
> to help with diagnostic, not to replace throwing exception,
