[jira] [Commented] (SENTRY-1512) Refactor the database transaction management
[
https://issues.apache.org/jira/browse/SENTRY-1512?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15617030#comment-15617030
]
Alexander Kolbasov commented on SENTRY-1512:
[~colinma] While reviewing the port I noticed a problem with the original code.
It only affects tests, not the core behavior.
In DelegateSentryStore.getPrivilegesByProvider() you call
{code}
if (groups != null) {
trimmedRoles.addAll(delegate.getRoleNamesForGroups(groups));
}
{code}
But delegate.getRoleNamesForGroups(groups)) is executed inside its own
transaction manager which is wrong.
This should be fixed (and there may be other cases as well). Also I think that
we can add assert in the transaction manager which will trigger an exception
when current transaction is active.
> Refactor the database transaction management
>
>
> Key: SENTRY-1512
> URL: https://issues.apache.org/jira/browse/SENTRY-1512
> Project: Sentry
> Issue Type: Improvement
>Reporter: Colin Ma
>Assignee: Colin Ma
> Attachments: SENTRY-1512.001.patch
>
>
> The update for SENTRY-1422 refactor the database transaction management, this
> also should be ported to master.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[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&focusedCommentId=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 reviewer
[jira] [Created] (SENTRY-1513) Avoid two splits in PathsUpdate.java
Sravya Tirukkovalur created SENTRY-1513:
---
Summary: Avoid two splits in PathsUpdate.java
Key: SENTRY-1513
URL: https://issues.apache.org/jira/browse/SENTRY-1513
Project: Sentry
Issue Type: Improvement
Reporter: Sravya Tirukkovalur
Priority: Trivial
{code}
if(uriPath.split("^/").length < 2) {
throw new SentryMalformedPathException("Path part of uri does
not seem right, was expecting a non empty path" +
": path = " + uriPath + ", uri=" + uri);
}
return Lists.newArrayList(uriPath.split("^/")[1].split("/"));
{code}
Also, correct the spelling in the same file
URIs with non hdfs *schemee* will just be ignore
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Updated] (SENTRY-1513) Avoid two splits in PathsUpdate.java
[
https://issues.apache.org/jira/browse/SENTRY-1513?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Sravya Tirukkovalur updated SENTRY-1513:
Labels: newbie (was: )
> Avoid two splits in PathsUpdate.java
>
>
> Key: SENTRY-1513
> URL: https://issues.apache.org/jira/browse/SENTRY-1513
> Project: Sentry
> Issue Type: Improvement
>Reporter: Sravya Tirukkovalur
>Priority: Trivial
> Labels: newbie
>
> {code}
> if(uriPath.split("^/").length < 2) {
> throw new SentryMalformedPathException("Path part of uri does
> not seem right, was expecting a non empty path" +
> ": path = " + uriPath + ", uri=" + uri);
> }
> return Lists.newArrayList(uriPath.split("^/")[1].split("/"));
> {code}
> Also, correct the spelling in the same file
> URIs with non hdfs *schemee* will just be ignore
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[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&focusedCommentId=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.
--
Thi
[jira] [Commented] (SENTRY-1377) improve handling of failures, both in tests and after-test cleanup, in TestHDFSIntegration.java
[
https://issues.apache.org/jira/browse/SENTRY-1377?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616833#comment-15616833
]
Hadoop QA commented on SENTRY-1377:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12822366/SENTRY-1377.003.patch
against master.
{color:red}Overall:{color} -1 due to an error
{color:red}ERROR:{color} failed to apply patch (exit code 1):
The patch does not appear to apply with p0, p1, or p2
Console output:
https://builds.apache.org/job/PreCommit-SENTRY-Build/2114/console
This message is automatically generated.
> improve handling of failures, both in tests and after-test cleanup, in
> TestHDFSIntegration.java
> ---
>
> Key: SENTRY-1377
> URL: https://issues.apache.org/jira/browse/SENTRY-1377
> Project: Sentry
> Issue Type: Sub-task
> Components: Sentry
>Affects Versions: 1.8.0
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Minor
> Fix For: 1.8.0
>
> Attachments: SENTRY-1377.001.patch, SENTRY-1377.002.patch,
> SENTRY-1377.003.patch
>
>
> 1. TestHDFSIntegration.java should provide best-attempt cleanup in
> cleanAfterTest() method. Currently, if any cleanup operation fails, the rest
> of cleanup code is not executed. Cleanup logic would not normally fail if
> corresponding test succeeds. But if some do not, short-circuiting some
> cleanup logic tends to cascade secondary failures which complicate
> troubleshooting.
> 2. Test methods would short-circuit closing database Connection's and
> Statement's if the test logic fails. Moreover, some test methods have
> multiple initializations of Connection ans Statement, but only one
> stmt.close(); conn.close(); pair at the end. Correct pattern for each
> distinct {Connection,Statement} pair would be:
> conn = null;
> stmt = null;
> try {
> . test logic here, including conn and stmt initialization ...
> } finally { safeClose(conn, stmt); }
> private static safeClose(Connection conn, Statement stmt) {
> if (stmt != null) try { stmt.close(); } catch (Exception ignore) {}
> if (conn != null) try { conn.close(); } catch (Exception ignore) {}
> }
> Testing jdbc driver implementation is not in the scope of HDFS integration
> tests, so ignoring Connection.close() and Statement.close() failures should
> not be a concern for this test class.
> 3. Retry logic uses recursion in some places, as in startHiveServer2(),
> verifyQuery(), verifyOnAllSubDirs. Better to implement it via straightforward
> retry loop. Exception stack trace is more confusing than it needs to be in
> case of recursive calls. Plus, with NUM_RETRIES == 10, at least
> theoretically, it creates running out of stack as an unnecessary failure
> mechanism.
> 4. startHiveServer2() ignores hiveServer2.start() call failure, only logging
> info message.
> 5. Starting hiveServer2 and Hive metastore in separate threads and then
> keeping those threads alive seems unnecessary, since both servers' start()
> methods create servers running on their own threads anyway. It effectively
> leads to ignoring the start() method failure for both servers. Also, it
> leaves no guarantee that hiveServer2 will be started after the Hive metastore
> - both start from independent threads with no cross-thread coordination
> mechanism in place.
> 6. miniDFS.getFileSystem().mkdirs(tmpHDFSDir) call missing in several places
> 7. Thread.sleep() missing in multiple places between HiveServer2 SQL calls
> changing permissions and verifyOnAllSubDirs() calls verifying that those
> changes took effect.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Updated] (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:all-tabpanel
]
Vadim Spector updated SENTRY-1508:
--
Attachment: SENTRY-1508.001.patch
> 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 sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Updated] (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:all-tabpanel
]
Vadim Spector updated SENTRY-1508:
--
Attachment: (was: SENTRY-1508.001.patch)
> 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 can be generated to only support
> synchronous initialization. This would lead to a much simpler and robust
> implementation.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Updated] (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:all-tabpanel
]
Vadim Spector updated SENTRY-1508:
--
Attachment: SENTRY-1508.001.patch
> 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 sent by Atlassian JIRA
(v6.3.4#6332)
[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&focusedCommentId=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,
[jira] [Updated] (SENTRY-1489) Categorize e2e tests into slow and regular tests, so that can adapt the timeout and etc.
[ https://issues.apache.org/jira/browse/SENTRY-1489?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Anne Yu updated SENTRY-1489: Resolution: Fixed Fix Version/s: (was: sentry-ha-redesign) Status: Resolved (was: Patch Available) > Categorize e2e tests into slow and regular tests, so that can adapt the > timeout and etc. > > > Key: SENTRY-1489 > URL: https://issues.apache.org/jira/browse/SENTRY-1489 > Project: Sentry > Issue Type: Test > Components: Sentry >Affects Versions: 1.8.0, sentry-ha-redesign >Reporter: Anne Yu >Assignee: Anne Yu > Fix For: 1.8.0 > > Attachments: SENTRY-1489.0.patch, SENTRY-1489.1.patch, > SENTRY-1489.2.patch > > -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (SENTRY-1489) Categorize e2e tests into slow and regular tests, so that can adapt the timeout and etc.
[ https://issues.apache.org/jira/browse/SENTRY-1489?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616812#comment-15616812 ] Anne Yu commented on SENTRY-1489: - Sure. > Categorize e2e tests into slow and regular tests, so that can adapt the > timeout and etc. > > > Key: SENTRY-1489 > URL: https://issues.apache.org/jira/browse/SENTRY-1489 > Project: Sentry > Issue Type: Test > Components: Sentry >Affects Versions: 1.8.0, sentry-ha-redesign >Reporter: Anne Yu >Assignee: Anne Yu > Fix For: 1.8.0, sentry-ha-redesign > > Attachments: SENTRY-1489.0.patch, SENTRY-1489.1.patch, > SENTRY-1489.2.patch > > -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (SENTRY-1489) Categorize e2e tests into slow and regular tests, so that can adapt the timeout and etc.
[ https://issues.apache.org/jira/browse/SENTRY-1489?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616794#comment-15616794 ] Sravya Tirukkovalur commented on SENTRY-1489: - Cool, shall we resolve this then? > Categorize e2e tests into slow and regular tests, so that can adapt the > timeout and etc. > > > Key: SENTRY-1489 > URL: https://issues.apache.org/jira/browse/SENTRY-1489 > Project: Sentry > Issue Type: Test > Components: Sentry >Affects Versions: 1.8.0, sentry-ha-redesign >Reporter: Anne Yu >Assignee: Anne Yu > Fix For: 1.8.0, sentry-ha-redesign > > Attachments: SENTRY-1489.0.patch, SENTRY-1489.1.patch, > SENTRY-1489.2.patch > > -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Updated] (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:all-tabpanel
]
Vadim Spector updated SENTRY-1508:
--
Attachment: (was: SENTRY-1508.001.patch)
> 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 can be generated to only support
> synchronous initialization. This would lead to a much simpler and robust
> implementation.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (SENTRY-1512) Refactor the database transaction management
[ https://issues.apache.org/jira/browse/SENTRY-1512?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616780#comment-15616780 ] Sravya Tirukkovalur commented on SENTRY-1512: - Thanks [~colinma]! Is this essentially a cherry-pick of Sentry-1422? Or did we have to do anything differently on master? > Refactor the database transaction management > > > Key: SENTRY-1512 > URL: https://issues.apache.org/jira/browse/SENTRY-1512 > Project: Sentry > Issue Type: Improvement >Reporter: Colin Ma >Assignee: Colin Ma > Attachments: SENTRY-1512.001.patch > > > The update for SENTRY-1422 refactor the database transaction management, this > also should be ported to master. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (SENTRY-1489) Categorize e2e tests into slow and regular tests, so that can adapt the timeout and etc.
[ https://issues.apache.org/jira/browse/SENTRY-1489?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616777#comment-15616777 ] Anne Yu commented on SENTRY-1489: - [~sravya], it takes a little time to sync, see here: https://github.com/apache/sentry/commit/b124675c0d571a4b85026d5390a7758092e04842 > Categorize e2e tests into slow and regular tests, so that can adapt the > timeout and etc. > > > Key: SENTRY-1489 > URL: https://issues.apache.org/jira/browse/SENTRY-1489 > Project: Sentry > Issue Type: Test > Components: Sentry >Affects Versions: 1.8.0, sentry-ha-redesign >Reporter: Anne Yu >Assignee: Anne Yu > Fix For: 1.8.0, sentry-ha-redesign > > Attachments: SENTRY-1489.0.patch, SENTRY-1489.1.patch, > SENTRY-1489.2.patch > > -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (SENTRY-1377) improve handling of failures, both in tests and after-test cleanup, in TestHDFSIntegration.java
[
https://issues.apache.org/jira/browse/SENTRY-1377?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616764#comment-15616764
]
Vadim Spector commented on SENTRY-1377:
---
[~sravya], this patch is out-of-date, since we've decided to re-do it on top of
the latest code that split TestHDFSIntegration into several classes. Still on
my todo list.
> improve handling of failures, both in tests and after-test cleanup, in
> TestHDFSIntegration.java
> ---
>
> Key: SENTRY-1377
> URL: https://issues.apache.org/jira/browse/SENTRY-1377
> Project: Sentry
> Issue Type: Sub-task
> Components: Sentry
>Affects Versions: 1.8.0
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Minor
> Fix For: 1.8.0
>
> Attachments: SENTRY-1377.001.patch, SENTRY-1377.002.patch,
> SENTRY-1377.003.patch
>
>
> 1. TestHDFSIntegration.java should provide best-attempt cleanup in
> cleanAfterTest() method. Currently, if any cleanup operation fails, the rest
> of cleanup code is not executed. Cleanup logic would not normally fail if
> corresponding test succeeds. But if some do not, short-circuiting some
> cleanup logic tends to cascade secondary failures which complicate
> troubleshooting.
> 2. Test methods would short-circuit closing database Connection's and
> Statement's if the test logic fails. Moreover, some test methods have
> multiple initializations of Connection ans Statement, but only one
> stmt.close(); conn.close(); pair at the end. Correct pattern for each
> distinct {Connection,Statement} pair would be:
> conn = null;
> stmt = null;
> try {
> . test logic here, including conn and stmt initialization ...
> } finally { safeClose(conn, stmt); }
> private static safeClose(Connection conn, Statement stmt) {
> if (stmt != null) try { stmt.close(); } catch (Exception ignore) {}
> if (conn != null) try { conn.close(); } catch (Exception ignore) {}
> }
> Testing jdbc driver implementation is not in the scope of HDFS integration
> tests, so ignoring Connection.close() and Statement.close() failures should
> not be a concern for this test class.
> 3. Retry logic uses recursion in some places, as in startHiveServer2(),
> verifyQuery(), verifyOnAllSubDirs. Better to implement it via straightforward
> retry loop. Exception stack trace is more confusing than it needs to be in
> case of recursive calls. Plus, with NUM_RETRIES == 10, at least
> theoretically, it creates running out of stack as an unnecessary failure
> mechanism.
> 4. startHiveServer2() ignores hiveServer2.start() call failure, only logging
> info message.
> 5. Starting hiveServer2 and Hive metastore in separate threads and then
> keeping those threads alive seems unnecessary, since both servers' start()
> methods create servers running on their own threads anyway. It effectively
> leads to ignoring the start() method failure for both servers. Also, it
> leaves no guarantee that hiveServer2 will be started after the Hive metastore
> - both start from independent threads with no cross-thread coordination
> mechanism in place.
> 6. miniDFS.getFileSystem().mkdirs(tmpHDFSDir) call missing in several places
> 7. Thread.sleep() missing in multiple places between HiveServer2 SQL calls
> changing permissions and verifyOnAllSubDirs() calls verifying that those
> changes took effect.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (SENTRY-1489) Categorize e2e tests into slow and regular tests, so that can adapt the timeout and etc.
[ https://issues.apache.org/jira/browse/SENTRY-1489?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616768#comment-15616768 ] Sravya Tirukkovalur commented on SENTRY-1489: - [~anneyu] I dont see this on master? > Categorize e2e tests into slow and regular tests, so that can adapt the > timeout and etc. > > > Key: SENTRY-1489 > URL: https://issues.apache.org/jira/browse/SENTRY-1489 > Project: Sentry > Issue Type: Test > Components: Sentry >Affects Versions: 1.8.0, sentry-ha-redesign >Reporter: Anne Yu >Assignee: Anne Yu > Fix For: 1.8.0, sentry-ha-redesign > > Attachments: SENTRY-1489.0.patch, SENTRY-1489.1.patch, > SENTRY-1489.2.patch > > -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Comment Edited] (SENTRY-1377) improve handling of failures, both in tests and after-test cleanup, in TestHDFSIntegration.java
[
https://issues.apache.org/jira/browse/SENTRY-1377?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616755#comment-15616755
]
Sravya Tirukkovalur edited comment on SENTRY-1377 at 10/28/16 10:16 PM:
[~vspector] Looks like the patch cannot be applied?
was (Author: sravya):
[~vspector] Looks like the patch cannot be applied? Feel free to change the
status back to patch available once this is ready for review.
> improve handling of failures, both in tests and after-test cleanup, in
> TestHDFSIntegration.java
> ---
>
> Key: SENTRY-1377
> URL: https://issues.apache.org/jira/browse/SENTRY-1377
> Project: Sentry
> Issue Type: Sub-task
> Components: Sentry
>Affects Versions: 1.8.0
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Minor
> Fix For: 1.8.0
>
> Attachments: SENTRY-1377.001.patch, SENTRY-1377.002.patch,
> SENTRY-1377.003.patch
>
>
> 1. TestHDFSIntegration.java should provide best-attempt cleanup in
> cleanAfterTest() method. Currently, if any cleanup operation fails, the rest
> of cleanup code is not executed. Cleanup logic would not normally fail if
> corresponding test succeeds. But if some do not, short-circuiting some
> cleanup logic tends to cascade secondary failures which complicate
> troubleshooting.
> 2. Test methods would short-circuit closing database Connection's and
> Statement's if the test logic fails. Moreover, some test methods have
> multiple initializations of Connection ans Statement, but only one
> stmt.close(); conn.close(); pair at the end. Correct pattern for each
> distinct {Connection,Statement} pair would be:
> conn = null;
> stmt = null;
> try {
> . test logic here, including conn and stmt initialization ...
> } finally { safeClose(conn, stmt); }
> private static safeClose(Connection conn, Statement stmt) {
> if (stmt != null) try { stmt.close(); } catch (Exception ignore) {}
> if (conn != null) try { conn.close(); } catch (Exception ignore) {}
> }
> Testing jdbc driver implementation is not in the scope of HDFS integration
> tests, so ignoring Connection.close() and Statement.close() failures should
> not be a concern for this test class.
> 3. Retry logic uses recursion in some places, as in startHiveServer2(),
> verifyQuery(), verifyOnAllSubDirs. Better to implement it via straightforward
> retry loop. Exception stack trace is more confusing than it needs to be in
> case of recursive calls. Plus, with NUM_RETRIES == 10, at least
> theoretically, it creates running out of stack as an unnecessary failure
> mechanism.
> 4. startHiveServer2() ignores hiveServer2.start() call failure, only logging
> info message.
> 5. Starting hiveServer2 and Hive metastore in separate threads and then
> keeping those threads alive seems unnecessary, since both servers' start()
> methods create servers running on their own threads anyway. It effectively
> leads to ignoring the start() method failure for both servers. Also, it
> leaves no guarantee that hiveServer2 will be started after the Hive metastore
> - both start from independent threads with no cross-thread coordination
> mechanism in place.
> 6. miniDFS.getFileSystem().mkdirs(tmpHDFSDir) call missing in several places
> 7. Thread.sleep() missing in multiple places between HiveServer2 SQL calls
> changing permissions and verifyOnAllSubDirs() calls verifying that those
> changes took effect.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Updated] (SENTRY-1377) improve handling of failures, both in tests and after-test cleanup, in TestHDFSIntegration.java
[
https://issues.apache.org/jira/browse/SENTRY-1377?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Sravya Tirukkovalur updated SENTRY-1377:
Status: Patch Available (was: Open)
> improve handling of failures, both in tests and after-test cleanup, in
> TestHDFSIntegration.java
> ---
>
> Key: SENTRY-1377
> URL: https://issues.apache.org/jira/browse/SENTRY-1377
> Project: Sentry
> Issue Type: Sub-task
> Components: Sentry
>Affects Versions: 1.8.0
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Minor
> Fix For: 1.8.0
>
> Attachments: SENTRY-1377.001.patch, SENTRY-1377.002.patch,
> SENTRY-1377.003.patch
>
>
> 1. TestHDFSIntegration.java should provide best-attempt cleanup in
> cleanAfterTest() method. Currently, if any cleanup operation fails, the rest
> of cleanup code is not executed. Cleanup logic would not normally fail if
> corresponding test succeeds. But if some do not, short-circuiting some
> cleanup logic tends to cascade secondary failures which complicate
> troubleshooting.
> 2. Test methods would short-circuit closing database Connection's and
> Statement's if the test logic fails. Moreover, some test methods have
> multiple initializations of Connection ans Statement, but only one
> stmt.close(); conn.close(); pair at the end. Correct pattern for each
> distinct {Connection,Statement} pair would be:
> conn = null;
> stmt = null;
> try {
> . test logic here, including conn and stmt initialization ...
> } finally { safeClose(conn, stmt); }
> private static safeClose(Connection conn, Statement stmt) {
> if (stmt != null) try { stmt.close(); } catch (Exception ignore) {}
> if (conn != null) try { conn.close(); } catch (Exception ignore) {}
> }
> Testing jdbc driver implementation is not in the scope of HDFS integration
> tests, so ignoring Connection.close() and Statement.close() failures should
> not be a concern for this test class.
> 3. Retry logic uses recursion in some places, as in startHiveServer2(),
> verifyQuery(), verifyOnAllSubDirs. Better to implement it via straightforward
> retry loop. Exception stack trace is more confusing than it needs to be in
> case of recursive calls. Plus, with NUM_RETRIES == 10, at least
> theoretically, it creates running out of stack as an unnecessary failure
> mechanism.
> 4. startHiveServer2() ignores hiveServer2.start() call failure, only logging
> info message.
> 5. Starting hiveServer2 and Hive metastore in separate threads and then
> keeping those threads alive seems unnecessary, since both servers' start()
> methods create servers running on their own threads anyway. It effectively
> leads to ignoring the start() method failure for both servers. Also, it
> leaves no guarantee that hiveServer2 will be started after the Hive metastore
> - both start from independent threads with no cross-thread coordination
> mechanism in place.
> 6. miniDFS.getFileSystem().mkdirs(tmpHDFSDir) call missing in several places
> 7. Thread.sleep() missing in multiple places between HiveServer2 SQL calls
> changing permissions and verifyOnAllSubDirs() calls verifying that those
> changes took effect.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Updated] (SENTRY-1377) improve handling of failures, both in tests and after-test cleanup, in TestHDFSIntegration.java
[
https://issues.apache.org/jira/browse/SENTRY-1377?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Sravya Tirukkovalur updated SENTRY-1377:
Status: Open (was: Patch Available)
> improve handling of failures, both in tests and after-test cleanup, in
> TestHDFSIntegration.java
> ---
>
> Key: SENTRY-1377
> URL: https://issues.apache.org/jira/browse/SENTRY-1377
> Project: Sentry
> Issue Type: Sub-task
> Components: Sentry
>Affects Versions: 1.8.0
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Minor
> Fix For: 1.8.0
>
> Attachments: SENTRY-1377.001.patch, SENTRY-1377.002.patch,
> SENTRY-1377.003.patch
>
>
> 1. TestHDFSIntegration.java should provide best-attempt cleanup in
> cleanAfterTest() method. Currently, if any cleanup operation fails, the rest
> of cleanup code is not executed. Cleanup logic would not normally fail if
> corresponding test succeeds. But if some do not, short-circuiting some
> cleanup logic tends to cascade secondary failures which complicate
> troubleshooting.
> 2. Test methods would short-circuit closing database Connection's and
> Statement's if the test logic fails. Moreover, some test methods have
> multiple initializations of Connection ans Statement, but only one
> stmt.close(); conn.close(); pair at the end. Correct pattern for each
> distinct {Connection,Statement} pair would be:
> conn = null;
> stmt = null;
> try {
> . test logic here, including conn and stmt initialization ...
> } finally { safeClose(conn, stmt); }
> private static safeClose(Connection conn, Statement stmt) {
> if (stmt != null) try { stmt.close(); } catch (Exception ignore) {}
> if (conn != null) try { conn.close(); } catch (Exception ignore) {}
> }
> Testing jdbc driver implementation is not in the scope of HDFS integration
> tests, so ignoring Connection.close() and Statement.close() failures should
> not be a concern for this test class.
> 3. Retry logic uses recursion in some places, as in startHiveServer2(),
> verifyQuery(), verifyOnAllSubDirs. Better to implement it via straightforward
> retry loop. Exception stack trace is more confusing than it needs to be in
> case of recursive calls. Plus, with NUM_RETRIES == 10, at least
> theoretically, it creates running out of stack as an unnecessary failure
> mechanism.
> 4. startHiveServer2() ignores hiveServer2.start() call failure, only logging
> info message.
> 5. Starting hiveServer2 and Hive metastore in separate threads and then
> keeping those threads alive seems unnecessary, since both servers' start()
> methods create servers running on their own threads anyway. It effectively
> leads to ignoring the start() method failure for both servers. Also, it
> leaves no guarantee that hiveServer2 will be started after the Hive metastore
> - both start from independent threads with no cross-thread coordination
> mechanism in place.
> 6. miniDFS.getFileSystem().mkdirs(tmpHDFSDir) call missing in several places
> 7. Thread.sleep() missing in multiple places between HiveServer2 SQL calls
> changing permissions and verifyOnAllSubDirs() calls verifying that those
> changes took effect.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (SENTRY-1377) improve handling of failures, both in tests and after-test cleanup, in TestHDFSIntegration.java
[
https://issues.apache.org/jira/browse/SENTRY-1377?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616755#comment-15616755
]
Sravya Tirukkovalur commented on SENTRY-1377:
-
[~vspector] Looks like the patch cannot be applied? Feel free to change the
status back to patch available once this is ready for review.
> improve handling of failures, both in tests and after-test cleanup, in
> TestHDFSIntegration.java
> ---
>
> Key: SENTRY-1377
> URL: https://issues.apache.org/jira/browse/SENTRY-1377
> Project: Sentry
> Issue Type: Sub-task
> Components: Sentry
>Affects Versions: 1.8.0
>Reporter: Vadim Spector
>Assignee: Vadim Spector
>Priority: Minor
> Fix For: 1.8.0
>
> Attachments: SENTRY-1377.001.patch, SENTRY-1377.002.patch,
> SENTRY-1377.003.patch
>
>
> 1. TestHDFSIntegration.java should provide best-attempt cleanup in
> cleanAfterTest() method. Currently, if any cleanup operation fails, the rest
> of cleanup code is not executed. Cleanup logic would not normally fail if
> corresponding test succeeds. But if some do not, short-circuiting some
> cleanup logic tends to cascade secondary failures which complicate
> troubleshooting.
> 2. Test methods would short-circuit closing database Connection's and
> Statement's if the test logic fails. Moreover, some test methods have
> multiple initializations of Connection ans Statement, but only one
> stmt.close(); conn.close(); pair at the end. Correct pattern for each
> distinct {Connection,Statement} pair would be:
> conn = null;
> stmt = null;
> try {
> . test logic here, including conn and stmt initialization ...
> } finally { safeClose(conn, stmt); }
> private static safeClose(Connection conn, Statement stmt) {
> if (stmt != null) try { stmt.close(); } catch (Exception ignore) {}
> if (conn != null) try { conn.close(); } catch (Exception ignore) {}
> }
> Testing jdbc driver implementation is not in the scope of HDFS integration
> tests, so ignoring Connection.close() and Statement.close() failures should
> not be a concern for this test class.
> 3. Retry logic uses recursion in some places, as in startHiveServer2(),
> verifyQuery(), verifyOnAllSubDirs. Better to implement it via straightforward
> retry loop. Exception stack trace is more confusing than it needs to be in
> case of recursive calls. Plus, with NUM_RETRIES == 10, at least
> theoretically, it creates running out of stack as an unnecessary failure
> mechanism.
> 4. startHiveServer2() ignores hiveServer2.start() call failure, only logging
> info message.
> 5. Starting hiveServer2 and Hive metastore in separate threads and then
> keeping those threads alive seems unnecessary, since both servers' start()
> methods create servers running on their own threads anyway. It effectively
> leads to ignoring the start() method failure for both servers. Also, it
> leaves no guarantee that hiveServer2 will be started after the Hive metastore
> - both start from independent threads with no cross-thread coordination
> mechanism in place.
> 6. miniDFS.getFileSystem().mkdirs(tmpHDFSDir) call missing in several places
> 7. Thread.sleep() missing in multiple places between HiveServer2 SQL calls
> changing permissions and verifyOnAllSubDirs() calls verifying that those
> changes took effect.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (SENTRY-1489) Categorize e2e tests into slow and regular tests, so that can adapt the timeout and etc.
[
https://issues.apache.org/jira/browse/SENTRY-1489?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616652#comment-15616652
]
Hadoop QA commented on SENTRY-1489:
---
Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12835875/SENTRY-1489.2.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/2113/console
This message is automatically generated.
> Categorize e2e tests into slow and regular tests, so that can adapt the
> timeout and etc.
>
>
> Key: SENTRY-1489
> URL: https://issues.apache.org/jira/browse/SENTRY-1489
> Project: Sentry
> Issue Type: Test
> Components: Sentry
>Affects Versions: 1.8.0, sentry-ha-redesign
>Reporter: Anne Yu
>Assignee: Anne Yu
> Fix For: 1.8.0, sentry-ha-redesign
>
> Attachments: SENTRY-1489.0.patch, SENTRY-1489.1.patch,
> SENTRY-1489.2.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (SENTRY-1489) Categorize e2e tests into slow and regular tests, so that can adapt the timeout and etc.
[ https://issues.apache.org/jira/browse/SENTRY-1489?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616634#comment-15616634 ] Anne Yu commented on SENTRY-1489: - Thank you LiLi. commit b124675c0d571a4b85026d5390a7758092e04842 Author: Anne Yu Date: Fri Oct 28 13:44:06 2016 -0700 SENTRY-1489: Categorize e2e tests into slow and regular tests, so that can adapt the timeout and etc. (Anne Yu, reviewed by LiLi) > Categorize e2e tests into slow and regular tests, so that can adapt the > timeout and etc. > > > Key: SENTRY-1489 > URL: https://issues.apache.org/jira/browse/SENTRY-1489 > Project: Sentry > Issue Type: Test > Components: Sentry >Affects Versions: 1.8.0, sentry-ha-redesign >Reporter: Anne Yu >Assignee: Anne Yu > Fix For: 1.8.0, sentry-ha-redesign > > Attachments: SENTRY-1489.0.patch, SENTRY-1489.1.patch, > SENTRY-1489.2.patch > > -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Updated] (SENTRY-1489) Categorize e2e tests into slow and regular tests, so that can adapt the timeout and etc.
[ https://issues.apache.org/jira/browse/SENTRY-1489?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Anne Yu updated SENTRY-1489: Attachment: SENTRY-1489.2.patch > Categorize e2e tests into slow and regular tests, so that can adapt the > timeout and etc. > > > Key: SENTRY-1489 > URL: https://issues.apache.org/jira/browse/SENTRY-1489 > Project: Sentry > Issue Type: Test > Components: Sentry >Affects Versions: 1.8.0, sentry-ha-redesign >Reporter: Anne Yu >Assignee: Anne Yu > Fix For: 1.8.0, sentry-ha-redesign > > Attachments: SENTRY-1489.0.patch, SENTRY-1489.1.patch, > SENTRY-1489.2.patch > > -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (SENTRY-1497) create a sentry scale test tool to add various objects and privileges into Sentry and HMS
[ https://issues.apache.org/jira/browse/SENTRY-1497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616231#comment-15616231 ] Anne Yu commented on SENTRY-1497: - Yes, I think so. I am doing more experiments using this tool now, will submit minor improvements later. Then cherry-pick to sentry-ha-redesign branch. > create a sentry scale test tool to add various objects and privileges into > Sentry and HMS > - > > Key: SENTRY-1497 > URL: https://issues.apache.org/jira/browse/SENTRY-1497 > Project: Sentry > Issue Type: Test > Components: Sentry >Affects Versions: 1.8.0, sentry-ha-redesign >Reporter: Anne Yu >Assignee: Anne Yu > Fix For: 1.8.0, sentry-ha-redesign > > Attachments: SENTRY-1497.0.patch, SENTRY-1497.1.patch, > SENTRY-1497.2.patch, SENTRY-1497.3.patch, SENTRY-1497.4.patch > > -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[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&focusedCommentId=15616210#comment-15616210
]
Sravya Tirukkovalur commented on SENTRY-1508:
-
Just to update here: Sentry1260 and Sentry-1270 have been committed.
> 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
> implemen
[jira] [Commented] (SENTRY-1497) create a sentry scale test tool to add various objects and privileges into Sentry and HMS
[ https://issues.apache.org/jira/browse/SENTRY-1497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15616209#comment-15616209 ] Li Li commented on SENTRY-1497: --- Thanks [~anneyu]! Do we also need this in sentry-ha-redesign branch? > create a sentry scale test tool to add various objects and privileges into > Sentry and HMS > - > > Key: SENTRY-1497 > URL: https://issues.apache.org/jira/browse/SENTRY-1497 > Project: Sentry > Issue Type: Test > Components: Sentry >Affects Versions: 1.8.0, sentry-ha-redesign >Reporter: Anne Yu >Assignee: Anne Yu > Fix For: 1.8.0, sentry-ha-redesign > > Attachments: SENTRY-1497.0.patch, SENTRY-1497.1.patch, > SENTRY-1497.2.patch, SENTRY-1497.3.patch, SENTRY-1497.4.patch > > -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (SENTRY-1512) Refactor the database transaction management
[ https://issues.apache.org/jira/browse/SENTRY-1512?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15614549#comment-15614549 ] Colin Ma commented on SENTRY-1512: -- I think it's usual to have the different JIRAs for the different branches. > Refactor the database transaction management > > > Key: SENTRY-1512 > URL: https://issues.apache.org/jira/browse/SENTRY-1512 > Project: Sentry > Issue Type: Improvement >Reporter: Colin Ma >Assignee: Colin Ma > Attachments: SENTRY-1512.001.patch > > > The update for SENTRY-1422 refactor the database transaction management, this > also should be ported to master. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
