You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Vamsee Yarlagadda <va...@cloudera.com> on 2017/01/03 19:46:47 UTC
Re: Review Request 54525: SENTRY-1541/SENTRY-1582: Additional
comments to
clarify the intent of string manipulation methods in SentryStore.java
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/
-----------------------------------------------------------
(Updated Jan. 3, 2017, 7:46 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
Summary (updated)
-----------------
SENTRY-1541/SENTRY-1582: Additional comments to clarify the intent of string manipulation methods in SentryStore.java
Repository: sentry
Description
-------
Provides more information on why some of the string conversions are required in SentryStore.java
Diffs
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 868e67720196f443cd281ec4e80ad552bf86e569
Diff: https://reviews.apache.org/r/54525/diff/
Testing
-------
```bash
vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
...
-------------------------------------------------------
T E S T S
-------------------------------------------------------
Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
Results :
Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 44.421s
[INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
[INFO] Final Memory: 67M/687M
[INFO] ------------------------------------------------------------------------
```
Thanks,
Vamsee Yarlagadda
Re: Review Request 54525: SENTRY-1541/SENTRY-1582: Additional
comments to
clarify the intent of string manipulation methods in SentryStore.java
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/#review160428
-----------------------------------------------------------
Fix it, then Ship it!
A few nits, otherwise good comments.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1720)
<https://reviews.apache.org/r/54525/#comment231567>
The comment ('function to check if...' gives an impression that this is a predicate.)
I think it should say something like
Convert different forms of empty strings to @NULL_COL and return all other input strings unmodified.
Possible empty strings:
- null
- empty string
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1722)
<https://reviews.apache.org/r/54525/#comment231568>
Please use <p> between paragraphs.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1729)
<https://reviews.apache.org/r/54525/#comment231569>
return original string if it is non-empty and @NULL_COL for empty strings.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1736)
<https://reviews.apache.org/r/54525/#comment231570>
See above for comments for toNULLCol
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1752)
<https://reviews.apache.org/r/54525/#comment231572>
s is not a column value - it is any string, and can be null.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1753)
<https://reviews.apache.org/r/54525/#comment231571>
@return True if the input string represents a NULL string - when it is null, empty or equals @NULL_COL
- Alexander Kolbasov
On Jan. 3, 2017, 7:46 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2017, 7:46 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Provides more information on why some of the string conversions are required in SentryStore.java
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 868e67720196f443cd281ec4e80ad552bf86e569
>
> Diff: https://reviews.apache.org/r/54525/diff/
>
>
> Testing
> -------
>
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
> ...
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
>
> Results :
>
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] ------------------------------------------------------------------------
> ```
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 54525: SENTRY-1541/SENTRY-1582: Additional
comments to
clarify the intent of string manipulation methods in SentryStore.java
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/#review160429
-----------------------------------------------------------
Fix it, then Ship it!
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1723)
<https://reviews.apache.org/r/54525/#comment231574>
Please use HTML formatting for lists -
<ul>
<li>Something</li>
<li>Something else</li>
</ul>
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1742)
<https://reviews.apache.org/r/54525/#comment231575>
Please use HTML formatting.
- Alexander Kolbasov
On Jan. 3, 2017, 8:02 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2017, 8:02 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Provides more information on why some of the string conversions are required in SentryStore.java
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 868e67720196f443cd281ec4e80ad552bf86e569
>
> Diff: https://reviews.apache.org/r/54525/diff/
>
>
> Testing
> -------
>
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
> ...
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
>
> Results :
>
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] ------------------------------------------------------------------------
> ```
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 54525: SENTRY-1541/SENTRY-1582: Additional
comments to
clarify the intent of string manipulation methods in SentryStore.java
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/#review160430
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Kolbasov
On Jan. 3, 2017, 8:15 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2017, 8:15 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Provides more information on why some of the string conversions are required in SentryStore.java
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 868e67720196f443cd281ec4e80ad552bf86e569
>
> Diff: https://reviews.apache.org/r/54525/diff/
>
>
> Testing
> -------
>
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
> ...
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
>
> Results :
>
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] ------------------------------------------------------------------------
> ```
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 54525: SENTRY-1541/SENTRY-1582: Additional
comments to
clarify the intent of string manipulation methods in SentryStore.java
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/
-----------------------------------------------------------
(Updated Jan. 3, 2017, 8:15 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
Changes
-------
Was already on it. Used HTML Formatting.
Repository: sentry
Description
-------
Provides more information on why some of the string conversions are required in SentryStore.java
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 868e67720196f443cd281ec4e80ad552bf86e569
Diff: https://reviews.apache.org/r/54525/diff/
Testing
-------
```bash
vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
...
-------------------------------------------------------
T E S T S
-------------------------------------------------------
Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
Results :
Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 44.421s
[INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
[INFO] Final Memory: 67M/687M
[INFO] ------------------------------------------------------------------------
```
Thanks,
Vamsee Yarlagadda
Re: Review Request 54525: SENTRY-1541/SENTRY-1582: Additional
comments to
clarify the intent of string manipulation methods in SentryStore.java
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/
-----------------------------------------------------------
(Updated Jan. 3, 2017, 8:02 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
Changes
-------
Addressed Sasha's comments.
Repository: sentry
Description
-------
Provides more information on why some of the string conversions are required in SentryStore.java
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 868e67720196f443cd281ec4e80ad552bf86e569
Diff: https://reviews.apache.org/r/54525/diff/
Testing
-------
```bash
vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
...
-------------------------------------------------------
T E S T S
-------------------------------------------------------
Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
Results :
Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 44.421s
[INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
[INFO] Final Memory: 67M/687M
[INFO] ------------------------------------------------------------------------
```
Thanks,
Vamsee Yarlagadda