Re: Review Request 64955: SENTRY-2109: Fix the logic of identifying HMS out of Sync

2018-02-01 Thread Vadim Spector via Review Board
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64955/#review196651 --- Ship it! Ship It! - Vadim Spector On Jan. 30, 2018, 1:08

Re: Review Request 64955: SENTRY-2109: Fix the logic of identifying HMS out of Sync

2018-01-26 Thread Vadim Spector via Review Board
> On Jan. 25, 2018, 11:42 p.m., Vadim Spector wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java > > Line 235 (original), 270 (patched) > > <https://reviews.apache.org/r/64955/diff/4-5/?file=19

Re: Review Request 64955: SENTRY-2109: Fix the logic of identifying HMS out of Sync

2018-01-25 Thread Vadim Spector via Review Board
tId()); return false; } Note "cache miss" in the second log. Cache miss is what we try to avoid for better performance. It may happen sometimes, but if there are too many "cache miss" messages, it's useful to know. - Vadim Spector On J

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

2018-01-25 Thread Vadim Spector via Review Board
268/diff/3/?file=1944703#file1944703line246> > > > > If you don't think that you can use time because we do not know how > > much time it may take to execute transaction, then the whole point of this > > JIRA is moot and we shouldn't fix it in the first place. > &

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

2018-01-25 Thread Vadim Spector via Review Board
On Jan. 23, 2018, 11:40 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65268/ > --- > > (Updated Jan. 23, 2018, 11:40 p.m.) &g

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

2018-01-23 Thread Vadim Spector via Review Board
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65268/#review196083 --- Ship it! Ship It! - Vadim Spector On Jan. 23, 2018, 11:40

Re: Review Request 65268: SENTRY-1904: TransactionManager should limit the max time spent by transaction retry

2018-01-23 Thread Vadim Spector via Review Board
if the exception is thrown, it would be logged, not re-thrown from the parent catch {} case? - Vadim Spector On Jan. 23, 2018, 6:43 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply,

Re: Review Request 64955: SENTRY-2109: Fix the logic of identifying HMS out of Sync

2018-01-23 Thread Vadim Spector via Review Board
> On Jan. 22, 2018, 8:49 p.m., Vadim Spector wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java > > Lines 67 (patched) > > <https://reviews.apache.org/r/64955/diff/4/?file=1943745#file1943745line68>

Re: Review Request 64955: SENTRY-2109: Fix the logic of identifying HMS out of Sync

2018-01-22 Thread Vadim Spector via Review Board
> On Jan. 22, 2018, 8:49 p.m., Vadim Spector wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java > > Lines 67 (patched) > > <https://reviews.apache.org/r/64955/diff/4/?file=1943745#file1943745line68>

Re: Review Request 64955: SENTRY-2109: Fix the logic of identifying HMS out of Sync

2018-01-22 Thread Vadim Spector via Review Board
optimization - no need for this DB call if the ID > max(storedId) - Vadim Spector On Jan. 22, 2018, 5:40 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated

Re: Review Request 64955: SENTRY-2109: Fix the logic of identifying HMS out of Sync

2018-01-18 Thread Vadim Spector via Review Board
can only change from false to true. Can it change from true to false? If yes, where can it happen? If not, why? Please, add comment on that. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java Line 206 (original) <https://reviews.apache.o

Re: Review Request 64820: SENTRY-2106: Remove sentry dependency on HMS table NOTIFICATION_SEQUENCE when checking if Sentry is out-of-sync with HMS

2018-01-18 Thread Vadim Spector via Review Board
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64820/#review195750 --- Ship it! Ship It! - Vadim Spector On Jan. 16, 2018, 3:50

Re: Review Request 64955: SENTRY-2109: Fix the logic of identifying HMS out of Sync

2018-01-04 Thread Vadim Spector via Review Board
> On Jan. 4, 2018, 10:40 p.m., Vadim Spector wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Line 205 (original), 206 (patched) > > <https://reviews.apache.org/r/64955/diff/1/?file=1930844#file1930844line20

Re: Review Request 64955: SENTRY-2109: Fix the logic of identifying HMS out of Sync

2018-01-04 Thread Vadim Spector via Review Board
alue stands for, since it obviously plays important role in the new logic - Vadim Spector On Jan. 4, 2018, 8:22 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated

Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot retry logic

2017-12-25 Thread Vadim Spector via Review Board
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64820/#review194500 --- Ship it! Ship It! - Vadim Spector On Dec. 25, 2017, 10:49

Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot retry logic

2017-12-24 Thread Vadim Spector via Review Board
t; clause with verify()'s for all mock objects? Post-sentry-ahead state is an important part to test too. - Vadim Spector On Dec. 24, 2017, 1:50 a.m., Arjun Mishra wrote: > > --

Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot retry logic

2017-12-23 Thread Vadim Spector via Review Board
ying to recover from "Sentry-ahead" situation. Which is when notificationSyncRetryCount != 0. Therefore, this log should be inside the "if (notificationSyncRetryCount != 0)" clause, it seems. - Vadim Spector On Dec. 24, 2017, 1:50 a.m., Arjun Mishra wrote: &g

Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot retry logic

2017-12-23 Thread Vadim Spector via Review Board
/sentry/service/thrift/HMSFollower.java Lines 542 (patched) <https://reviews.apache.org/r/64820/#comment273243> this method is used from TestHMSFollower, which is the same package, so it can be declared package-private instead of public - Vadim Spector On Dec. 22, 2017, 11:34 p.m., Arjun

Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot retry logic

2017-12-23 Thread Vadim Spector via Review Board
nother reason to call isSentryAhead() from this method. - Vadim Spector On Dec. 22, 2017, 11:34 p.m., Arjun Mishra wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.

Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot retry logic

2017-12-23 Thread Vadim Spector via Review Board
lt in fewer diffs and will be easier to undrestand what's exactly changed. - Vadim Spector On Dec. 22, 2017, 11:34 p.m., Arjun Mishra wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https:

Re: Review Request 64820: SENTRY-2106: Sentry ahead full snapshot retry logic

2017-12-23 Thread Vadim Spector via Review Board
Lines 228 (patched) <https://reviews.apache.org/r/64820/#comment273239> nit: extra line - Vadim Spector On Dec. 22, 2017, 11:34 p.m., Arjun Mishra wrote: > > --- > This is an automatically generated e-mail. To reply, vi

Re: Review Request 64661: SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles()

2017-12-19 Thread Vadim Spector via Review Board
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64661/#review194174 --- Ship it! Ship It! - Vadim Spector On Dec. 19, 2017, 8:04

Re: Review Request 64661: SENTRY-1944: Optimize DelegateSentryStore.getGroupsByRoles()

2017-12-18 Thread Vadim Spector via Review Board
roles with identical groups for each role. I'd define roles role1 and role2 with partially iverlapping groups for each, i.e. a couple of groups defined only for role1, another couple only for role2, and another couple for both role1 and role2. - Vadim Spector On Dec. 18, 201

Re: Review Request 64452: SENTRY-2091: User-based Privilege is broken by SENTRY-769

2017-12-15 Thread Vadim Spector via Review Board
), 368 (patched) <https://reviews.apache.org/r/64452/#comment272684> Ditto: do we know CREATE fails for the right reason (user1 lacking privilege), not because the table already exists? - Vadim Spector On Dec. 14, 2017, 8:54 p.m.,

Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

2017-12-13 Thread Vadim Spector via Review Board
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64545/#review193707 --- Ship it! Ship It! - Vadim Spector On Dec. 13, 2017, 5:45

Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

2017-12-12 Thread Vadim Spector via Review Board
ll these fields logged, but in reality we rarely complain that there is too much information in the logs; usually the opposite. But whichever way you decide, it's already a good improvement. - Vadim Spector On Dec. 12,

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-06 Thread Vadim Spector via Review Board
/sentry/service/thrift/CounterWait.java Line 164 (original), 165 (patched) <https://reviews.apache.org/r/63881/#comment271537> Question for this and similar parts of code: will it be common for old and new values to have gaps, and if not, would it make sense to warn() about it? - Vadim S

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-05 Thread Vadim Spector via Review Board
line432> > > > > Use `ID = {}", event.getEventId`. > > Also, do we want to show the actual failure here or it will be printed > > elsewhere? > > Vadim Spector wrote: > For some reason processNotifications() is declared public, yet it is only

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-05 Thread Vadim Spector via Review Board
- On Dec. 5, 2017, 9:02 p.m., Arjun Mishra wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63881/ > ---

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-05 Thread Vadim Spector via Review Board
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63881/#review192934 --- Ship it! Ship It! - Vadim Spector On Dec. 5, 2017, 9:02 p.m

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-12-05 Thread Vadim Spector via Review Board
c-html/org/apache/commons/lang3/exception/ExceptionUtils.html It addresses weird case where exception chain forms a linked list with the loop, which would result in infinite loop. In theory shoud never happen, but not impossible. - Vadim Spector On Dec. 5, 2017, 5:32 p.m., Arjun Mishra

Re: Review Request 63881: SENTRY-2040: When getting Snapshots from HMS we need more logging around cases when a snapshot is not being received

2017-11-30 Thread Vadim Spector via Review Board
getEventId() != (currentEventId+1)) { LOGGER.warn(...); } - Vadim Spector On Nov. 30, 2017, 10:41 p.m., Arjun Mishra wrote: > > --- > This is an automatically generated e-mail. To reply, v

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-28 Thread Vadim Spector via Review Board
> On Nov. 28, 2017, 10:16 p.m., Vadim Spector wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > > Line 2514 (original), 2513 (patched) > > <https://reviews.apache.org/r/6359

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-28 Thread Vadim Spector via Review Board
e should do just that and test the resuts by passing non-canonicalized paths and make sure canonicalization happened. Would like others to weigh in. - Vadim Spector On Nov. 27, 2017, 6:03 p.m., Arjun Mishra wrote: > > --

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-28 Thread Vadim Spector via Review Board
> On Nov. 28, 2017, 10:16 p.m., Vadim Spector wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > > Line 2514 (original), 2513 (patched) > > <https://reviews.apache.org/r/6359

Re: Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

2017-11-20 Thread Vadim Spector via Review Board
otNull(msg_1, y.getName()); Assert.assertEquals(x.toLowerCase(), y.getName().toLowerCase()); where the last line will throw an exception telling exactly the expected value and the actual value, if they are different. This is far more helpful. - Vadim Spector On Nov. 21, 2017, 3:32 a.

Re: Review Request 63886: SENTRY-2047: isTableEmptyCore method in SentryStore has references to MAuthzPathsMapping when it should be generic

2017-11-16 Thread Vadim Spector via Review Board
> On Nov. 16, 2017, 8:14 p.m., Vadim Spector wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Line 3251 (original), 3251 (patched) > > <https://reviews.apache.org/r/63886/diff/1/?file=189

Re: Review Request 63886: SENTRY-2047: isTableEmptyCore method in SentryStore has references to MAuthzPathsMapping when it should be generic

2017-11-16 Thread Vadim Spector via Review Board
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63886/#review191251 --- Ship it! Ship It! - Vadim Spector On Nov. 16, 2017, 7:29

Re: Review Request 63645: SENTRY-2032: Leading Slashes need to removed when creating HMS path entries

2017-11-16 Thread Vadim Spector via Review Board
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63645/#review191247 --- Ship it! Ship It! - Vadim Spector On Nov. 16, 2017, 8:38

Re: Review Request 63886: SENTRY-2047: isTableEmptyCore method in SentryStore has references to MAuthzPathsMapping when it should be generic

2017-11-16 Thread Vadim Spector via Review Board
t we add such a test now? - Vadim Spector On Nov. 16, 2017, 7:29 p.m., Arjun Mishra wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https:

Re: Review Request 63645: SENTRY-2032: Leading Slashes need to removed when creating HMS path entries

2017-11-16 Thread Vadim Spector via Review Board
yle also allows you to define upfront all possible paths as an array of strings, and then do all assertions inside the loop. More compact and easy to add new test cases. - Vadim Spector On Nov. 8, 2017, 3:03 p.m., Arjun Mishra wrote: > >

Re: Review Request 63646: SENTRY-2035: Metrics should move to destination atomically

2017-11-07 Thread Vadim Spector via Review Board
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63646/#review190414 --- Ship it! Ship It! - Vadim Spector On Nov. 7, 2017, 9:47 p.m

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-06 Thread Vadim Spector via Review Board
> On Nov. 6, 2017, 10:11 p.m., Vadim Spector wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > > Lines 2453 (patched) > > <https://reviews.apache.org/r/63596/diff/1/?file=1882251#file1882251

Re: Review Request 63596: SENTRY-1951 - Old SentryStore.retrieveFullPathsImage() should be removed

2017-11-06 Thread Vadim Spector via Review Board
ich does _not_ begin with "/". Why is that? What has changed? If it's correct, it's worth at least one sentence of javadoc. - Vadim Spector On Nov. 6, 2017, 9:52 p.m., Arjun Mishra wrote: > > --- > This is an a

Re: Review Request 63086: SENTRY-1993: StringIndexOutOfBoundsException in HMSPathsDumper.java

2017-10-20 Thread Vadim Spector via Review Board
e fix. - Vadim Spector On Oct. 20, 2017, 7:14 p.m., Misha Dmitriev wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.a

Re: Review Request 63086: SENTRY-1993: StringIndexOutOfBoundsException in HMSPathsDumper.java

2017-10-18 Thread Vadim Spector via Review Board
> On Oct. 18, 2017, 2:44 p.m., Vadim Spector wrote: > > Ship It! I'll proceed with commit today - Vadim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63086

Re: Review Request 63086: SENTRY-1993: StringIndexOutOfBoundsException in HMSPathsDumper.java

2017-10-18 Thread Vadim Spector via Review Board
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63086/#review188492 --- Ship it! Ship It! - Vadim Spector On Oct. 17, 2017, 7:12

Re: Review Request 62496: SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system

2017-10-03 Thread Vadim Spector
object, so there should be a placeholder for it in a format string. The signature for Throwable is "error(String msg, Throwable t)", which would require "LOGGER.error("Unable to delete " + path, e);" - Vadim Spector On Sept.

Re: Review Request 62112: SENTRY-1915 Sentry is doing a lot of work to convert list of paths to HMSPaths structure

2017-09-06 Thread Vadim Spector
- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62112/ > --- > > (Updated Sept. 6, 2017, 6:48 p.m.) > > > Review request for sentry, Arjun Mishra, Brian Towles, Na Li,

Re: Review Request 62007: SENTRY-1909 Improvements for memory usage when full path snapshot is sent from Sentry to NN

2017-09-01 Thread Vadim Spector
> On Sept. 1, 2017, 7 p.m., Vadim Spector wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateModifier.java > > Line 142 (original), 143 (patched) > > <https://reviews.apache.org/r/62007/diff/3/?file=18

Re: Review Request 62007: SENTRY-1909 Improvements for memory usage when full path snapshot is sent from Sentry to NN

2017-09-01 Thread Vadim Spector
bclass, depending on the circumstances. Neither seems to apply. Besides, cast exception is always cleaner to be thrown up the call chain. - Vadim Spector On Aug. 31, 2017, 4:14 p.m., Alexander Kolbasov wrote: > > --- > This is a

Re: Review Request 61827: SENTRY-1892: Reduce memory consumption of HMSPath and TPathEntry

2017-08-23 Thread Vadim Spector
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61827/#review183680 --- Ship it! Ship It! - Vadim Spector On Aug. 23, 2017, 10:08

Re: Review Request 61827: SENTRY-1892: Reduce memory consumption of HMSPath and TPathEntry

2017-08-23 Thread Vadim Spector
el the dump creation event, plus some stats like # of redundant strings, which could be supplied by DupDetector. - Vadim Spector On Aug. 23, 2017, 7:29 p.m., Misha Dmitriev wrote: > > --- > This is an automatically generated e

Re: Review Request 61823: SENTRY-1896 - Optimize retrieving roles for groups

2017-08-22 Thread Vadim Spector
gt; (Updated Aug. 22, 2017, 9:58 p.m.) > > > Review request for sentry, Alexander Kolbasov, Vamsee Yarlagadda, and Vadim > Spector. > > > Repository: sentry > > > Description > --- > > Right now when we get privileges from sentry, we pass in a provid

Re: Review Request 61113: SENTRY-1755 Add HMSFollower per-operation metrics

2017-07-25 Thread Vadim Spector
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61113/#review181383 --- Ship it! Ship It! - Vadim Spector On July 25, 2017, 4:32

Re: Review Request 61113: SENTRY-1755 Add HMSFollower per-operation metrics

2017-07-25 Thread Vadim Spector
/sentry/service/thrift/NotificationProcessor.java Lines 194 (patched) <https://reviews.apache.org/r/61113/#comment256917> Any chance of valueOf() returning null here? - Vadim Spector On July 25, 2017, 4:32 p.m., Alexander Kolbasov

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

2017-05-19 Thread Vadim Spector
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59317/#review175553 --- Ship it! Ship It! - Vadim Spector On May 19, 2017, 1:18 a.m

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

2017-05-19 Thread Vadim Spector
> On May 19, 2017, 6:26 p.m., Vadim Spector wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/UserGroupInformationInitializer.java > > Line 36 (original), 40 (patched) > > <https://reviews.apache.org/r/59317/diff/4-5/?file=1

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

2017-05-19 Thread Vadim Spector
/core/common/transport/UserGroupInformationInitializer.java Line 36 (original), 40 (patched) <https://reviews.apache.org/r/59317/#comment248946> Hmm ... how dit it end up reverting the fix? - Vadim Spector On May 19, 2017, 1:18 a.m., kalyan kumar kalvagadda

Re: Review Request 59317: SENTRY-1736 Generic service client should support Kerberos

2017-05-18 Thread Vadim Spector
riginal), 64 (patched) <https://reviews.apache.org/r/59317/#comment248875> "this.conf" is only initialized on the next line; should use "conf" instead of "this.conf" - or swap these two lines. - Vadim Spector On May 18

Re: Review Request 56954: SENTRY-1639 Refactor the usage of configuration constants related to transport

2017-03-13 Thread Vadim Spector
provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java Line 75 (original), 77 (patched) <https://reviews.apache.org/r/56954/#comment241071> could youi add javadoc on thread safety of this class? -

Re: Review Request 57219: SENTRY-1601 Implement HMS Notification barrier on the server side

2017-03-03 Thread Vadim Spector
whether to address them or not to the author. - Vadim Spector On March 3, 2017, 3:08 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache

Re: Review Request 57219: SENTRY-1601 Implement HMS Notification barrier on the server side

2017-03-03 Thread Vadim Spector
nthreads+1 threads, and run a "post counter update" loop on that additional thread. - Vadim Spector On March 3, 2017, 3:08 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit

Re: Review Request 57219: SENTRY-1601 Implement HMS Notification barrier on the server side

2017-03-03 Thread Vadim Spector
> On March 3, 2017, 10:13 p.m., Vadim Spector wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestCounterWait.java > > Lines 55 (patched) > > <https://reviews.apache.org/r/57219/diff/5/?file=1655286#file1655286line55> >

Re: Review Request 57219: SENTRY-1601 Implement HMS Notification barrier on the server side

2017-03-03 Thread Vadim Spector
) loop: cdl.countDown(); Another thought, if CounterWait had getNumWaiters() method, it would be more robust (less chances of flaky tests) to do smth. like this instead of sleep(100): while (waiter.getNumWaiters() < nthreads) { sleep(10); } - Vadim Spector On March 3

Re: Review Request 57219: SENTRY-1601 Implement HMS Notification barrier on the server side

2017-03-03 Thread Vadim Spector
/sentry/service/thrift/TestCounterWait.java Lines 49 (patched) <https://reviews.apache.org/r/57219/#comment239832> You mean "pair of threads per each value", right? - Vadim Spector On March 3, 2017, 3:08 a.m., Alexande

Re: Review Request 57219: SENTRY-1601 Implement HMS Notification barrier on the server side

2017-03-02 Thread Vadim Spector
Am I missing something? - Vadim Spector On March 2, 2017, 5:26 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.

Re: Review Request 57219: SENTRY-1601 Implement HMS Notification barrier on the server side

2017-03-02 Thread Vadim Spector
barrier's purpose? - Vadim Spector On March 2, 2017, 5:26 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.a

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-24 Thread Vadim Spector
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56481/#review166783 --- Ship it! - Vadim Spector On Feb. 23, 2017, 8:30 p.m

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
> On Feb. 22, 2017, 5:54 a.m., Vadim Spector wrote: > > Ship It! I think it was important to bring up the two important issues: a) broken functionality when using the same field in the query more than once, and b) re-ordering query elements to always put nested queries at the end. A

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56481/#review166321 --- Ship it! Ship It! - Vadim Spector On Feb. 20, 2017, 5:46

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
> On Feb. 21, 2017, 9:02 p.m., Vadim Spector wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java, > > line 88 > > <https://reviews.apache.org/r/56481/diff/2/?file=1640442#file1640442line

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
> On Feb. 21, 2017, 11:38 p.m., Vadim Spector wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java, > > line 65 > > <https://reviews.apache.org/r/56481/diff/2/?file=1640442#file1640442li

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
> On Feb. 21, 2017, 11:38 p.m., Vadim Spector wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java, > > line 65 > > <https://reviews.apache.org/r/56481/diff/2/?file=1640442#file1640442li

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
> On Feb. 21, 2017, 11:38 p.m., Vadim Spector wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java, > > line 65 > > <https://reviews.apache.org/r/56481/diff/2/?file=1640442#file1640442li

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
> On Feb. 21, 2017, 9:02 p.m., Vadim Spector wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java, > > line 88 > > <https://reviews.apache.org/r/56481/diff/2/?file=1640442#file1640442line

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
> On Feb. 21, 2017, 8:09 p.m., Vadim Spector wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java, > > line 73 > > <https://reviews.apache.org/r/56481/diff/2/?file=1640442#file1640442line7

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
same field name appearing more than once, this problem would automatically be addressed, because the ":value" part would be controlled entirely by this class. - Vadim Spector On Feb. 20, 2017, 5:46 a.m., Alexander Kolbasov wrote: > >

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
> On Feb. 21, 2017, 11:38 p.m., Vadim Spector wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java, > > line 65 > > <https://reviews.apache.org/r/56481/diff/2/?file=1640442#file1640442li

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
omething? If there is a reason not to support the same field appearing in the query more than once, it needs to be documented and an exception should be thrown when someone tries to override an existing entry in "arguments

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java (line 82) <https://reviews.apache.org/r/56481/#comment238158> "for search given privilege" - grammar? - Vadim Spector On Feb. 20, 2017, 5:46 a.m., Alexande

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
/sentry/provider/db/service/persistent/QueryParamBuilder.java (line 31) <https://reviews.apache.org/r/56481/#comment238121> Could you, please, document thread (un)safety assumptions for this class? - Vadim Spector On Feb. 20, 2017, 5:46 a.m., Alexander Kolbasov

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
f the query creator code is clever enough. I'd recommend QueryParamBuilder to subclass QueryPart and be a container for a single List to include both simple query parts and nested queries, so they're kept in the order of their addition. - Vadim Spector On Feb. 20, 2017,

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
newChild(boolean isAndOp), to keep "enup Op" private. Also, do you think a new method for querying QueryParamBuilder for operation type could be useful? - Vadim Spector On Feb. 20, 2017, 5:46 a.m., Alexander Kolbasov wrote: > >

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
/sentry/provider/db/service/persistent/QueryParamBuilder.java (line 74) <https://reviews.apache.org/r/56481/#comment238105> Since ctor is declared package-private, perhaps should use QueryParamBuilder.newQueryParamBuilder() instead? - Vadim Spector On Feb. 20, 2017, 5:46 a.m., Ale

Re: Review Request 56481: SENTRY-1625: PrivilegeOperatePersistence can use QueryParamBuilder

2017-02-21 Thread Vadim Spector
p still aply (since now the Map is combination of ANDs and ORs)? - Vadim Spector On Feb. 20, 2017, 5:46 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://re

Re: Review Request 56356: SENTRY-1615 SentryStore should not allocate empty objects that are immediately returned

2017-02-07 Thread Vadim Spector
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56356/#review164566 --- Ship it! Ship It! - Vadim Spector On Feb. 7, 2017, 2:14 a.m

Re: Review Request 55246: SENTRY-1536: Refactor SentryStore transaction management to allow for extra transactions for a single permission update

2017-02-03 Thread Vadim Spector
> On Feb. 3, 2017, 10:59 p.m., Vadim Spector wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java, > > line 295 > > <https://reviews.apache.org/r/55246/diff/9/?file=1622992#file1622992line295> > > > > gen

Re: Review Request 55246: SENTRY-1536: Refactor SentryStore transaction management to allow for extra transactions for a single permission update

2017-02-03 Thread Vadim Spector
, that permsUpdater.handleUpdateNotification(update) can take "update" object with the sequence numbers out-of-order. Can it become a problem? - Vadim Spector On Feb. 3, 2017, 5:47 a.m., Hao Hao wrote: > > --- >

Re: Review Request 55246: SENTRY-1536: Refactor SentryStore transaction management to allow for extra transactions for a single permission update

2017-02-03 Thread Vadim Spector
> On Feb. 3, 2017, 7:12 p.m., Vadim Spector wrote: > > Hao, I'm not sure what "to allow for extra transactions for a single > > permission update" means, could you, please, clarify? Do you mean placing a single permision update inside a transaction (then I

Re: Review Request 55246: SENTRY-1536: Refactor SentryStore transaction management to allow for extra transactions for a single permission update

2017-02-03 Thread Vadim Spector
ions for a single permission update" means, could you, please, clarify? - Vadim Spector On Feb. 3, 2017, 5:47 a.m., Hao Hao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https:/

Re: Review Request 56134: SENTRY-1593 and SENTRY-1592 Implementing client failover for Generic policy clients and namenode clients

2017-01-31 Thread Vadim Spector
6134/#comment235212> why not use implicit loop? - Vadim Spector On Jan. 31, 2017, 8:39 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: >

Re: Review Request 55080: SENTRY-1428: Only leader should follow HMS updates

2017-01-03 Thread Vadim Spector
out partial updates - how do they happen - or do they happen at all? Hard to review the code not knowing what it needs to acomplish - javadoc would help. - Vadim Spector On Dec. 29, 2016, 9:56 a.m., Alexander Kolbasov wrote: > > -

Re: Review Request 54808: SENTRY-1526 Sentry processed stays alive after being killed

2016-12-16 Thread Vadim Spector
> On Dec. 16, 2016, 7:05 p.m., Vadim Spector wrote: > > It is probably ok, but ... > > a) Did you investigate why service stayed alive? Perhaps, some knowledge > > about the implementation and its defficiencies to be gained there. Some > > unaccounted runaway t

Re: Review Request 54808: SENTRY-1526 Sentry processed stays alive after being killed

2016-12-16 Thread Vadim Spector
> On Dec. 16, 2016, 7:05 p.m., Vadim Spector wrote: > > It is probably ok, but ... > > a) Did you investigate why service stayed alive? Perhaps, some knowledge > > about the implementation and its defficiencies to be gained there. Some > > unaccounted runaway t

Re: Review Request 54808: SENTRY-1526 Sentry processed stays alive after being killed

2016-12-16 Thread Vadim Spector
the case now. Are we ok with making that a requirement from now on? - Vadim Spector On Dec. 16, 2016, 7:23 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.

Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in SentryStore

2016-12-14 Thread Vadim Spector
other similar cases too. - Vadim Spector On Dec. 7, 2016, 11:42 p.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.

Re: Review Request 54445: SENTRY-1547 - It is possible to create a privilege with all empty fields

2016-12-14 Thread Vadim Spector
eed to decide if it's the right way). - Vadim Spector On Dec. 14, 2016, 10:23 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://rev

Re: Review Request 54445: SENTRY-1547 - It is possible to create a privilege with all empty fields

2016-12-06 Thread Vadim Spector
leading and trailing spaces if any? - Vadim Spector On Dec. 6, 2016, 10:32 p.m., kalyan kumar kalvagadda wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://rev

Re: Review Request 54338: SENTRY-1515: Cleanup exception handling in SentryStore

2016-12-05 Thread Vadim Spector
ould print stack trace anyway - do we need e.printStackTrace()? - Vadim Spector On Dec. 3, 2016, 6:52 a.m., Alexander Kolbasov wrote: > > --- > This is an automatically generated e-mail. To reply,

  1   2   >