You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Anne Yu <an...@cloudera.com> on 2015/11/18 00:06:41 UTC
Review Request 40408: Add concurrent jdbc and thrift client tests to
ensure synchronization works fine
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40408/
-----------------------------------------------------------
Review request for sentry, Hao Hao, Li Li, Lenni Kuff, and Sravya Tirukkovalur.
Bugs: SENTRY-906
https://issues.apache.org/jira/browse/SENTRY-906
Repository: sentry
Description
-------
Add concurrent jdbc and thrift client tests to ensure synchronization works fine;
Diffs
-----
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc5daef
Diff: https://reviews.apache.org/r/40408/diff/
Testing
-------
I can successfully run locally this test.
Thanks,
Anne Yu
Re: Review Request 40408: Add concurrent jdbc and thrift client tests
to ensure synchronization works fine
Posted by Anne Yu <an...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40408/#review107216
-----------------------------------------------------------
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java (line 90)
<https://reviews.apache.org/r/40408/#comment166229>
Haohao, generally, thanks for your review. It helps the test automation.
For here, forgot to replace fixed number 5 with this var. Will change.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java (line 201)
<https://reviews.apache.org/r/40408/#comment166230>
numSuccess is an instance var, its setter and getter usually are called as setVarName and getVarName. So it's better we can keep the name here.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java (line 207)
<https://reviews.apache.org/r/40408/#comment166231>
Yeah, will remove extra spaces here.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java (line 229)
<https://reviews.apache.org/r/40408/#comment166234>
Can make it a more meaningful descriptions, such as, "Starting test to create database, table, role and partitions."
- Anne Yu
On Nov. 17, 2015, 11:06 p.m., Anne Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40408/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2015, 11:06 p.m.)
>
>
> Review request for sentry, Hao Hao, Li Li, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-906
> https://issues.apache.org/jira/browse/SENTRY-906
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Add concurrent jdbc and thrift client tests to ensure synchronization works fine;
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc5daef
>
> Diff: https://reviews.apache.org/r/40408/diff/
>
>
> Testing
> -------
>
> I can successfully run locally this test.
>
>
> Thanks,
>
> Anne Yu
>
>
Re: Review Request 40408: Add concurrent jdbc and thrift client tests
to ensure synchronization works fine
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40408/#review107107
-----------------------------------------------------------
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java (line 90)
<https://reviews.apache.org/r/40408/#comment166041>
when is 'len' used?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java (line 201)
<https://reviews.apache.org/r/40408/#comment166044>
change to autoIncreaseNumSuccess?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java (line 207)
<https://reviews.apache.org/r/40408/#comment166035>
"synchronized Throwable"(two spaces) to "synchronized Throwable", same as line 195,201,204.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java (line 229)
<https://reviews.apache.org/r/40408/#comment166043>
Add more info to be the specific task?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java (line 156)
<https://reviews.apache.org/r/40408/#comment166046>
API docs for this one will be good as well.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java (line 481)
<https://reviews.apache.org/r/40408/#comment166045>
Could you add a small docs for the api as well?
- Hao Hao
On Nov. 17, 2015, 11:06 p.m., Anne Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40408/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2015, 11:06 p.m.)
>
>
> Review request for sentry, Hao Hao, Li Li, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-906
> https://issues.apache.org/jira/browse/SENTRY-906
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Add concurrent jdbc and thrift client tests to ensure synchronization works fine;
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc5daef
>
> Diff: https://reviews.apache.org/r/40408/diff/
>
>
> Testing
> -------
>
> I can successfully run locally this test.
>
>
> Thanks,
>
> Anne Yu
>
>
Re: Review Request 40408: Add concurrent jdbc and thrift client tests
to ensure synchronization works fine
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40408/#review109016
-----------------------------------------------------------
Ship it!
Ship It!
- Hao Hao
On Dec. 4, 2015, 6:26 a.m., Anne Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40408/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2015, 6:26 a.m.)
>
>
> Review request for sentry, Hao Hao, Li Li, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-906
> https://issues.apache.org/jira/browse/SENTRY-906
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Add concurrent jdbc and thrift client tests to ensure synchronization works fine;
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-hive/pom.xml b70fe60
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java dc8c1eb
>
> Diff: https://reviews.apache.org/r/40408/diff/
>
>
> Testing
> -------
>
> I can successfully run locally this test.
>
>
> Thanks,
>
> Anne Yu
>
>
Re: Review Request 40408: Add concurrent jdbc and thrift client tests
to ensure synchronization works fine
Posted by Anne Yu <an...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40408/
-----------------------------------------------------------
(Updated Dec. 4, 2015, 6:26 a.m.)
Review request for sentry, Hao Hao, Li Li, Lenni Kuff, and Sravya Tirukkovalur.
Changes
-------
Addressed more code review comments from Hao Hao and Li Li.
Bugs: SENTRY-906
https://issues.apache.org/jira/browse/SENTRY-906
Repository: sentry
Description
-------
Add concurrent jdbc and thrift client tests to ensure synchronization works fine;
Diffs (updated)
-----
sentry-tests/sentry-tests-hive/pom.xml b70fe60
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java dc8c1eb
Diff: https://reviews.apache.org/r/40408/diff/
Testing
-------
I can successfully run locally this test.
Thanks,
Anne Yu
Re: Review Request 40408: Add concurrent jdbc and thrift client tests
to ensure synchronization works fine
Posted by Li Li <li...@cloudera.com>.
> On Dec. 3, 2015, 7:10 a.m., Li Li wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java, line 141
> > <https://reviews.apache.org/r/40408/diff/1/?file=1128920#file1128920line141>
> >
> > should we move stmt.close() and connection.close() inside of finally?
>
> Anne Yu wrote:
> Will move.
Thanks, Anne! Besides, should we finally close stmt and connection in other methods as well, eg. adminCleanUp, adminShowRole, adminGrant, createPartition, createDbTb ?
- Li
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40408/#review108787
-----------------------------------------------------------
On Nov. 17, 2015, 11:06 p.m., Anne Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40408/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2015, 11:06 p.m.)
>
>
> Review request for sentry, Hao Hao, Li Li, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-906
> https://issues.apache.org/jira/browse/SENTRY-906
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Add concurrent jdbc and thrift client tests to ensure synchronization works fine;
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc5daef
>
> Diff: https://reviews.apache.org/r/40408/diff/
>
>
> Testing
> -------
>
> I can successfully run locally this test.
>
>
> Thanks,
>
> Anne Yu
>
>
Re: Review Request 40408: Add concurrent jdbc and thrift client tests
to ensure synchronization works fine
Posted by Anne Yu <an...@cloudera.com>.
> On Dec. 3, 2015, 7:10 a.m., Li Li wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java, line 141
> > <https://reviews.apache.org/r/40408/diff/1/?file=1128920#file1128920line141>
> >
> > should we move stmt.close() and connection.close() inside of finally?
>
> Anne Yu wrote:
> Will move.
>
> Li Li wrote:
> Thanks, Anne! Besides, should we finally close stmt and connection in other methods as well, eg. adminCleanUp, adminShowRole, adminGrant, createPartition, createDbTb ?
Yeah, have noticed that also. Let me see if possible could do it.
- Anne
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40408/#review108787
-----------------------------------------------------------
On Nov. 17, 2015, 11:06 p.m., Anne Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40408/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2015, 11:06 p.m.)
>
>
> Review request for sentry, Hao Hao, Li Li, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-906
> https://issues.apache.org/jira/browse/SENTRY-906
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Add concurrent jdbc and thrift client tests to ensure synchronization works fine;
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc5daef
>
> Diff: https://reviews.apache.org/r/40408/diff/
>
>
> Testing
> -------
>
> I can successfully run locally this test.
>
>
> Thanks,
>
> Anne Yu
>
>
Re: Review Request 40408: Add concurrent jdbc and thrift client tests
to ensure synchronization works fine
Posted by Anne Yu <an...@cloudera.com>.
> On Dec. 3, 2015, 7:10 a.m., Li Li wrote:
> >
Thanks for your review.
> On Dec. 3, 2015, 7:10 a.m., Li Li wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java, line 191
> > <https://reviews.apache.org/r/40408/diff/1/?file=1128920#file1128920line191>
> >
> > How about use AtomicInteger which has better performance than synchrozied?
>From previous comments, seems using synchronized on a small api/class is cleaner than guard one variable. Maybe can keep it as it is.
> On Dec. 3, 2015, 7:10 a.m., Li Li wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java, line 141
> > <https://reviews.apache.org/r/40408/diff/1/?file=1128920#file1128920line141>
> >
> > should we move stmt.close() and connection.close() inside of finally?
Will move.
> On Dec. 3, 2015, 7:10 a.m., Li Li wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java, line 163
> > <https://reviews.apache.org/r/40408/diff/1/?file=1128920#file1128920line163>
> >
> > remove extral indention in line 163 and 166?
Will remove the whitespaces.
- Anne
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40408/#review108787
-----------------------------------------------------------
On Nov. 17, 2015, 11:06 p.m., Anne Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40408/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2015, 11:06 p.m.)
>
>
> Review request for sentry, Hao Hao, Li Li, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-906
> https://issues.apache.org/jira/browse/SENTRY-906
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Add concurrent jdbc and thrift client tests to ensure synchronization works fine;
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc5daef
>
> Diff: https://reviews.apache.org/r/40408/diff/
>
>
> Testing
> -------
>
> I can successfully run locally this test.
>
>
> Thanks,
>
> Anne Yu
>
>
Re: Review Request 40408: Add concurrent jdbc and thrift client tests
to ensure synchronization works fine
Posted by Li Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40408/#review108787
-----------------------------------------------------------
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java (line 141)
<https://reviews.apache.org/r/40408/#comment168268>
should we move stmt.close() and connection.close() inside of finally?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java (line 163)
<https://reviews.apache.org/r/40408/#comment168267>
remove extral indention in line 163 and 166?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java (line 191)
<https://reviews.apache.org/r/40408/#comment168269>
How about use AtomicInteger which has better performance than synchrozied?
- Li Li
On Nov. 17, 2015, 11:06 p.m., Anne Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40408/
> -----------------------------------------------------------
>
> (Updated Nov. 17, 2015, 11:06 p.m.)
>
>
> Review request for sentry, Hao Hao, Li Li, Lenni Kuff, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-906
> https://issues.apache.org/jira/browse/SENTRY-906
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Add concurrent jdbc and thrift client tests to ensure synchronization works fine;
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestConcurrentClients.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java cc5daef
>
> Diff: https://reviews.apache.org/r/40408/diff/
>
>
> Testing
> -------
>
> I can successfully run locally this test.
>
>
> Thanks,
>
> Anne Yu
>
>