[jira] [Commented] (SENTRY-1471) TestHDFSIntegrationBase.java implements HDFS ACL checking and query verification incorrectly
[ https://issues.apache.org/jira/browse/SENTRY-1471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15494783#comment-15494783 ] Hadoop QA commented on SENTRY-1471: --- Here are the results of testing the latest attachment https://issues.apache.org/jira/secure/attachment/12828748/SENTRY-1471.001.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.provider.db.generic.service.persistent.TestPrivilegeOperatePersistence Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/2025/console This message is automatically generated. > TestHDFSIntegrationBase.java implements HDFS ACL checking and query > verification incorrectly > > > Key: SENTRY-1471 > URL: https://issues.apache.org/jira/browse/SENTRY-1471 > Project: Sentry > Issue Type: Bug > Components: Sentry >Reporter: Vadim Spector >Assignee: Anne Yu > Attachments: SENTRY-1471.001.patch > > > verifyOnAllSubDirsHelper() in TestHDFSIntegrationBase.java has a bug. When > the number of failed retries exceeds max value (the "else" portion of "if > (retry > 0)" statement, it erroneously assigns hasSucceeded = true. It means > that verifyOnAllSubDirsHelper() never returns false. > Once the problem was fixed in a local development branch, several tests in > TestHDFSIntegrationAdvanced.java started failing. TestHDFSIntegrationEnd2End > tests are still ok. > Also, it is unfortunate that TestHDFSIntegrationBase() returns boolean > instead of throwing the original AssertionError which contains information > about expected vs. found ACL permissions - this information is invaluable for > debugging. The fix is easy - the to-be-fixed "else" portion is inside the > "catch (Throwable th)" clause, so it should just re-throw caught exception. > This makes "hasSucceeded" variable unnecessary - verifyOnAllSubDirsHelper() > should have "void" return type. > The same problem exists in verifyQuery(), also in > TestHDFSIntegrationBase.java. The "else" clause of "if (retry > 0)" also > assigns "isSucceeded = true;" leading to the same problem - it never fails. > Suggested solution would be the same - just re-throw the exception. It will > not only fix the problem, but also make debugging easier. Current code > "Assert.assertTrue(isSucceeded);" throws away valuable information about the > expected vs detected number of database rows. > Note: the above problems were introduced by SENTRY-1454. Suggested fixes > match the original code prior to SENTRY-1454. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (SENTRY-1471) TestHDFSIntegrationBase.java implements HDFS ACL checking and query verification incorrectly
[ https://issues.apache.org/jira/browse/SENTRY-1471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15494634#comment-15494634 ] Hadoop QA commented on SENTRY-1471: --- Here are the results of testing the latest attachment https://issues.apache.org/jira/secure/attachment/12828735/SENTRY-1471.001.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/2024/console This message is automatically generated. > TestHDFSIntegrationBase.java implements HDFS ACL checking and query > verification incorrectly > > > Key: SENTRY-1471 > URL: https://issues.apache.org/jira/browse/SENTRY-1471 > Project: Sentry > Issue Type: Bug > Components: Sentry >Reporter: Vadim Spector >Assignee: Anne Yu > Attachments: SENTRY-1471.001.patch > > > verifyOnAllSubDirsHelper() in TestHDFSIntegrationBase.java has a bug. When > the number of failed retries exceeds max value (the "else" portion of "if > (retry > 0)" statement, it erroneously assigns hasSucceeded = true. It means > that verifyOnAllSubDirsHelper() never returns false. > Once the problem was fixed in a local development branch, several tests in > TestHDFSIntegrationAdvanced.java started failing. TestHDFSIntegrationEnd2End > tests are still ok. > Also, it is unfortunate that TestHDFSIntegrationBase() returns boolean > instead of throwing the original AssertionError which contains information > about expected vs. found ACL permissions - this information is invaluable for > debugging. The fix is easy - the to-be-fixed "else" portion is inside the > "catch (Throwable th)" clause, so it should just re-throw caught exception. > This makes "hasSucceeded" variable unnecessary - verifyOnAllSubDirsHelper() > should have "void" return type. > The same problem exists in verifyQuery(), also in > TestHDFSIntegrationBase.java. The "else" clause of "if (retry > 0)" also > assigns "isSucceeded = true;" leading to the same problem - it never fails. > Suggested solution would be the same - just re-throw the exception. It will > not only fix the problem, but also make debugging easier. Current code > "Assert.assertTrue(isSucceeded);" throws away valuable information about the > expected vs detected number of database rows. > Note: the above problems were introduced by SENTRY-1454. Suggested fixes > match the original code prior to SENTRY-1454. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (SENTRY-1471) TestHDFSIntegrationBase.java implements HDFS ACL checking and query verification incorrectly
[ https://issues.apache.org/jira/browse/SENTRY-1471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15494416#comment-15494416 ] Vadim Spector commented on SENTRY-1471: --- Per conversation with [~anneyu] - disabling two unit tests testNonHDFSLocations() testAuthzObjOnPartitionMultipleTables(), both found broken after fixing the validation code - to be fixed in a separate JIRA. > TestHDFSIntegrationBase.java implements HDFS ACL checking and query > verification incorrectly > > > Key: SENTRY-1471 > URL: https://issues.apache.org/jira/browse/SENTRY-1471 > Project: Sentry > Issue Type: Bug > Components: Sentry >Reporter: Vadim Spector >Assignee: Anne Yu > Attachments: SENTRY-1471.001.patch > > > verifyOnAllSubDirsHelper() in TestHDFSIntegrationBase.java has a bug. When > the number of failed retries exceeds max value (the "else" portion of "if > (retry > 0)" statement, it erroneously assigns hasSucceeded = true. It means > that verifyOnAllSubDirsHelper() never returns false. > Once the problem was fixed in a local development branch, several tests in > TestHDFSIntegrationAdvanced.java started failing. TestHDFSIntegrationEnd2End > tests are still ok. > Also, it is unfortunate that TestHDFSIntegrationBase() returns boolean > instead of throwing the original AssertionError which contains information > about expected vs. found ACL permissions - this information is invaluable for > debugging. The fix is easy - the to-be-fixed "else" portion is inside the > "catch (Throwable th)" clause, so it should just re-throw caught exception. > This makes "hasSucceeded" variable unnecessary - verifyOnAllSubDirsHelper() > should have "void" return type. > The same problem exists in verifyQuery(), also in > TestHDFSIntegrationBase.java. The "else" clause of "if (retry > 0)" also > assigns "isSucceeded = true;" leading to the same problem - it never fails. > Suggested solution would be the same - just re-throw the exception. It will > not only fix the problem, but also make debugging easier. Current code > "Assert.assertTrue(isSucceeded);" throws away valuable information about the > expected vs detected number of database rows. > Note: the above problems were introduced by SENTRY-1454. Suggested fixes > match the original code prior to SENTRY-1454. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (SENTRY-1471) TestHDFSIntegrationBase.java implements HDFS ACL checking and query verification incorrectly
[ https://issues.apache.org/jira/browse/SENTRY-1471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15494413#comment-15494413 ] Hadoop QA commented on SENTRY-1471: --- Here are the results of testing the latest attachment https://issues.apache.org/jira/secure/attachment/12828721/SENTRY-1471.001.patch against master. {color:red}Overall:{color} -1 due to an error {color:red}ERROR:{color} failed to build with patch (exit code 1) Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/2022/console This message is automatically generated. > TestHDFSIntegrationBase.java implements HDFS ACL checking and query > verification incorrectly > > > Key: SENTRY-1471 > URL: https://issues.apache.org/jira/browse/SENTRY-1471 > Project: Sentry > Issue Type: Bug > Components: Sentry >Reporter: Vadim Spector >Assignee: Anne Yu > Attachments: SENTRY-1471.001.patch > > > verifyOnAllSubDirsHelper() in TestHDFSIntegrationBase.java has a bug. When > the number of failed retries exceeds max value (the "else" portion of "if > (retry > 0)" statement, it erroneously assigns hasSucceeded = true. It means > that verifyOnAllSubDirsHelper() never returns false. > Once the problem was fixed in a local development branch, several tests in > TestHDFSIntegrationAdvanced.java started failing. TestHDFSIntegrationEnd2End > tests are still ok. > Also, it is unfortunate that TestHDFSIntegrationBase() returns boolean > instead of throwing the original AssertionError which contains information > about expected vs. found ACL permissions - this information is invaluable for > debugging. The fix is easy - the to-be-fixed "else" portion is inside the > "catch (Throwable th)" clause, so it should just re-throw caught exception. > This makes "hasSucceeded" variable unnecessary - verifyOnAllSubDirsHelper() > should have "void" return type. > The same problem exists in verifyQuery(), also in > TestHDFSIntegrationBase.java. The "else" clause of "if (retry > 0)" also > assigns "isSucceeded = true;" leading to the same problem - it never fails. > Suggested solution would be the same - just re-throw the exception. It will > not only fix the problem, but also make debugging easier. Current code > "Assert.assertTrue(isSucceeded);" throws away valuable information about the > expected vs detected number of database rows. > Note: the above problems were introduced by SENTRY-1454. Suggested fixes > match the original code prior to SENTRY-1454. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (SENTRY-1471) TestHDFSIntegrationBase.java implements HDFS ACL checking and query verification incorrectly
[ https://issues.apache.org/jira/browse/SENTRY-1471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15494239#comment-15494239 ] Hadoop QA commented on SENTRY-1471: --- Here are the results of testing the latest attachment https://issues.apache.org/jira/secure/attachment/12828704/SENTRY-1471.001.patch against master. {color:red}Overall:{color} -1 due to an error {color:red}ERROR:{color} failed to build with patch (exit code 1) Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/2020/console This message is automatically generated. > TestHDFSIntegrationBase.java implements HDFS ACL checking and query > verification incorrectly > > > Key: SENTRY-1471 > URL: https://issues.apache.org/jira/browse/SENTRY-1471 > Project: Sentry > Issue Type: Bug > Components: Sentry >Reporter: Vadim Spector >Assignee: Anne Yu > Attachments: SENTRY-1471.001.patch > > > verifyOnAllSubDirsHelper() in TestHDFSIntegrationBase.java has a bug. When > the number of failed retries exceeds max value (the "else" portion of "if > (retry > 0)" statement, it erroneously assigns hasSucceeded = true. It means > that verifyOnAllSubDirsHelper() never returns false. > Once the problem was fixed in a local development branch, several tests in > TestHDFSIntegrationAdvanced.java started failing. TestHDFSIntegrationEnd2End > tests are still ok. > Also, it is unfortunate that TestHDFSIntegrationBase() returns boolean > instead of throwing the original AssertionError which contains information > about expected vs. found ACL permissions - this information is invaluable for > debugging. The fix is easy - the to-be-fixed "else" portion is inside the > "catch (Throwable th)" clause, so it should just re-throw caught exception. > This makes "hasSucceeded" variable unnecessary - verifyOnAllSubDirsHelper() > should have "void" return type. > The same problem exists in verifyQuery(), also in > TestHDFSIntegrationBase.java. The "else" clause of "if (retry > 0)" also > assigns "isSucceeded = true;" leading to the same problem - it never fails. > Suggested solution would be the same - just re-throw the exception. It will > not only fix the problem, but also make debugging easier. Current code > "Assert.assertTrue(isSucceeded);" throws away valuable information about the > expected vs detected number of database rows. > Note: the above problems were introduced by SENTRY-1454. Suggested fixes > match the original code prior to SENTRY-1454. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (SENTRY-1471) TestHDFSIntegrationBase.java implements HDFS ACL checking and query verification incorrectly
[ https://issues.apache.org/jira/browse/SENTRY-1471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15494222#comment-15494222 ] Vadim Spector commented on SENTRY-1471: --- Submitting the patch that fixes validation logic. a) testAuthzObjOnPartitionMultipleTables() is definitely broken - line 603, permissions not propagated to /tmp/external. Test marked as @Ignore'd b) testNonHDFSLocations() - in two places calls to verifyOnAllSubDirs() replace "FsAction.ALL" with "null" - - both mentioning SENTRY-546 as justification for code change. Well, they actually start failing, and switching back to "FsAction.ALL" fixes the test. c) after the above changes, all tests pass I suggest proceeding with this JIRA and opening another one, just to fix testAuthzObjOnPartitionMultipleTables() test. > TestHDFSIntegrationBase.java implements HDFS ACL checking and query > verification incorrectly > > > Key: SENTRY-1471 > URL: https://issues.apache.org/jira/browse/SENTRY-1471 > Project: Sentry > Issue Type: Bug > Components: Sentry >Reporter: Vadim Spector >Assignee: Anne Yu > Attachments: SENTRY-1471.001.patch > > > verifyOnAllSubDirsHelper() in TestHDFSIntegrationBase.java has a bug. When > the number of failed retries exceeds max value (the "else" portion of "if > (retry > 0)" statement, it erroneously assigns hasSucceeded = true. It means > that verifyOnAllSubDirsHelper() never returns false. > Once the problem was fixed in a local development branch, several tests in > TestHDFSIntegrationAdvanced.java started failing. TestHDFSIntegrationEnd2End > tests are still ok. > Also, it is unfortunate that TestHDFSIntegrationBase() returns boolean > instead of throwing the original AssertionError which contains information > about expected vs. found ACL permissions - this information is invaluable for > debugging. The fix is easy - the to-be-fixed "else" portion is inside the > "catch (Throwable th)" clause, so it should just re-throw caught exception. > This makes "hasSucceeded" variable unnecessary - verifyOnAllSubDirsHelper() > should have "void" return type. > The same problem exists in verifyQuery(), also in > TestHDFSIntegrationBase.java. The "else" clause of "if (retry > 0)" also > assigns "isSucceeded = true;" leading to the same problem - it never fails. > Suggested solution would be the same - just re-throw the exception. It will > not only fix the problem, but also make debugging easier. Current code > "Assert.assertTrue(isSucceeded);" throws away valuable information about the > expected vs detected number of database rows. > Note: the above problems were introduced by SENTRY-1454. Suggested fixes > match the original code prior to SENTRY-1454. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (SENTRY-1471) TestHDFSIntegrationBase.java implements HDFS ACL checking and query verification incorrectly
[ https://issues.apache.org/jira/browse/SENTRY-1471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15489405#comment-15489405 ] Anne Yu commented on SENTRY-1471: - [~vspec...@gmail.com], thanks for pointing out this issue. Since you've spent quite some time investigating the issue, do you have idea of after fix SENTRY-1471, why TestHDFSIntegrationAdvanced starts to fail? I guess this jira is meant to fix catch logic in verifyOnAllSubDirsHelper and verifyQuery APIs, but without fixing the test failures, this patch won't pass upstream-qa. Or do you have a jira filed already for real TestHDFSIntegrationAdvanced test failures? In that case, I can wait for it to be fixed. > TestHDFSIntegrationBase.java implements HDFS ACL checking and query > verification incorrectly > > > Key: SENTRY-1471 > URL: https://issues.apache.org/jira/browse/SENTRY-1471 > Project: Sentry > Issue Type: Bug > Components: Sentry >Reporter: Vadim Spector >Assignee: Anne Yu > > verifyOnAllSubDirsHelper() in TestHDFSIntegrationBase.java has a bug. When > the number of failed retries exceeds max value (the "else" portion of "if > (retry > 0)" statement, it erroneously assigns hasSucceeded = true. It means > that verifyOnAllSubDirsHelper() never returns false. > Once the problem was fixed in a local development branch, several tests in > TestHDFSIntegrationAdvanced.java started failing. TestHDFSIntegrationEnd2End > tests are still ok. > Also, it is unfortunate that TestHDFSIntegrationBase() returns boolean > instead of throwing the original AssertionError which contains information > about expected vs. found ACL permissions - this information is invaluable for > debugging. The fix is easy - the to-be-fixed "else" portion is inside the > "catch (Throwable th)" clause, so it should just re-throw caught exception. > This makes "hasSucceeded" variable unnecessary - verifyOnAllSubDirsHelper() > should have "void" return type. > The same problem exists in verifyQuery(), also in > TestHDFSIntegrationBase.java. The "else" clause of "if (retry > 0)" also > assigns "isSucceeded = true;" leading to the same problem - it never fails. > Suggested solution would be the same - just re-throw the exception. It will > not only fix the problem, but also make debugging easier. Current code > "Assert.assertTrue(isSucceeded);" throws away valuable information about the > expected vs detected number of database rows. > Note: the above problems were introduced by SENTRY-1454. Suggested fixes > match the original code prior to SENTRY-1454. -- This message was sent by Atlassian JIRA (v6.3.4#6332)