----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59720/#review177418 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo Lines 246 (patched) <https://reviews.apache.org/r/59720/#comment250954> intellij complains that key-cache-size is not allowed here. Looking at dtd I see: <!ELEMENT field (extension*, (array|collection|map)?, join?, embedded?, element?, key?, value?, order?, column*, foreign-key?, index?, unique?, extension*)> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 130 (patched) <https://reviews.apache.org/r/59720/#comment250991> Please add documentation sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2539 (patched) <https://reviews.apache.org/r/59720/#comment250994> It may be better to write simple case first: if (hmsImageID <= EMPTY_HMS_IMAGE_ID) { return Collections.emptyMap(); } and then continue without if statement. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2541 (patched) <https://reviews.apache.org/r/59720/#comment250992> imageId is long, but hmsImage is the class - are you sure this is the right query? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2535 (original), 2543 (patched) <https://reviews.apache.org/r/59720/#comment250993> We know that JDO returns List, so if you use Collection here instead of Iterable you'll save extra code executed in Iterable.isEmpty()/Iterable.size(), etc. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2546 (patched) <https://reviews.apache.org/r/59720/#comment250995> Same thing about reversing if statement and returning immediately sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2545 (original), 2559 (patched) <https://reviews.apache.org/r/59720/#comment250996> Add <p> for paragraph separation. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2561 (patched) <https://reviews.apache.org/r/59720/#comment250997> hms image, not hive image. Do we need to be that strict? Multiple images are ok - it is just a waste of DB resources. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2562 (patched) <https://reviews.apache.org/r/59720/#comment250998> if another image is present This prevents multiple transactions persisting different images. WHy would we get an exception? We use auto-incremented ID, so a new image will be happily created with a new ID. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2547 (original), 2565 (patched) <https://reviews.apache.org/r/59720/#comment251000> why do you want to < Paths < sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2574 (patched) <https://reviews.apache.org/r/59720/#comment251001> you can use known set size sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2555 (original), 2575 (patched) <https://reviews.apache.org/r/59720/#comment251003> Note that we can do this outside of transaction. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2580 (patched) <https://reviews.apache.org/r/59720/#comment251004> This is the only thing that should be inside a transaction sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2589 (patched) <https://reviews.apache.org/r/59720/#comment251005> There are no callers - so looks like it is never deleted. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2632 (patched) <https://reviews.apache.org/r/59720/#comment251006> what is 'desc'? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2577 (original), 2635 (patched) <https://reviews.apache.org/r/59720/#comment251007> Same comment about Iterable vs Collection sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2618 (original), 2675 (patched) <https://reviews.apache.org/r/59720/#comment251010> The image includes paths mappings which are huge and later we get them again with getMAuthzPathsMappingCore so looks like we are fetching it twice. Do we really need this? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2676 (patched) <https://reviews.apache.org/r/59720/#comment251008> This is a matter of style - in other place we use normal ordering currentHmsImage == null I personally prefer the natural ordering, but more important is to be cconsistent. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2677 (patched) <https://reviews.apache.org/r/59720/#comment251009> I think IllegalState exception is an unchecked exception - is it what we really want here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2622 (original), 2685 (patched) <https://reviews.apache.org/r/59720/#comment251012> What is the official style? Can we write this as else for ... { } or it isn't kosher? - Alexander Kolbasov On June 8, 2017, 5:57 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59720/ > ----------------------------------------------------------- > > (Updated June 8, 2017, 5:57 p.m.) > > > Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and > Vamsee Yarlagadda. > > > Bugs: SENTRY-1781 > https://issues.apache.org/jira/browse/SENTRY-1781 > > > Repository: sentry > > > Description > ------- > > This patch only adds an extra column to the MAuthzPathsMapping table that > contains an image identifier that will be used to detect full snapshots. This > patch does not increment the identifier yet, it only uses 0 for now. > > TIPS for reviewers: > > - package.jdo adds a new column name AUTHZ_IMG_ID to the AUTHZ_MAPS_MAPPING > table used to identify the paths that are part of a new hive snapshot. > > - MAuthzPathsMapping.java adds the the new column and API to handle JDO > requests. > > - SentryStore.java modifies all methods that interact with the snapshots. It > makes sure that > when persisting a new snapshot, then the authzImgID is incremented and the > old one is > deleted. Also it makes sure that retrieve/add/delete/rename/update uses the > current > authzImgID whenver they're called. > > - TestSentryStore.java adds unit tests to verify that all SentryStore API > work correctly > when a new hive snapshot is created. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java > f51894bb7e109c37997e7134e07a82f46c0a3c44 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MHiveMetastoreImage.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo > 459939b7600e05d57c7e11eefe20c2591cac0b34 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 37eb0b25e10ef69057599277aa5941ca05d52290 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 788062c12ab4e4180adf2d4b4a191e9b5c616f87 > > > Diff: https://reviews.apache.org/r/59720/diff/6/ > > > Testing > ------- > > All unit tests run locally, but there are some flaky tests on Jenkins no > related to this patch. > > > Thanks, > > Sergio Pena > >
