> On June 9, 2017, 6 a.m., Alexander Kolbasov wrote: > > 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/diff/6/?file=1744920#file1744920line2544> > > > > It may be better to write simple case first: > > > > if (hmsImageID <= EMPTY_HMS_IMAGE_ID) { > > return Collections.emptyMap(); > > } > > > > and then continue without if statement.
I did the fix, but why is this simple? There aren't more conditions that could make the code complex. I agree that sometimes this is required, but this code is easy to read. > On June 9, 2017, 6 a.m., Alexander Kolbasov wrote: > > 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/diff/6/?file=1744920#file1744920line2546> > > > > imageId is long, but hmsImage is the class - are you sure this is the > > right query? Yes. It is confusing, but that's how JDO allows me to filter the class. The HMS_IMAGE_ID from MHiveMetastoreImage is the primary key, so internally JDO will use that in the filter. > On June 9, 2017, 6 a.m., Alexander Kolbasov wrote: > > 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/diff/6/?file=1744920#file1744920line2566> > > > > hms image, not hive image. > > > > Do we need to be that strict? Multiple images are ok - it is just a > > waste of DB resources. This is a discussion I had offline with another developer. Seems it is not necessary to have and deal with multiple HMS images, so why do we? The code will not handle the prevention but the unique column created on MHiveMetastoreImage. The idea is that the HMSFollower will delete this MHiveMetastoreImage first, and in the next iteration, a new image will be persisted. This column can be removed, and things will work normally withou problems, but it is only to avoid this waste of DB resources. > On June 9, 2017, 6 a.m., Alexander Kolbasov wrote: > > 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/diff/6/?file=1744920#file1744920line2567> > > > > 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. See response above. > On June 9, 2017, 6 a.m., Alexander Kolbasov wrote: > > 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/diff/6/?file=1744920#file1744920line2570> > > > > why do you want to < Paths < I don't understand. This doc is already there. > On June 9, 2017, 6 a.m., Alexander Kolbasov wrote: > > 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/diff/6/?file=1744920#file1744920line2580> > > > > Note that we can do this outside of transaction. Good point. > On June 9, 2017, 6 a.m., Alexander Kolbasov wrote: > > 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/diff/6/?file=1744920#file1744920line2594> > > > > There are no callers - so looks like it is never deleted. I added it because it will be used by the HMSFollower class when detecting new full snapshots. The idea is to delete the current snapshot, and then persist it in different transactions. > On June 9, 2017, 6 a.m., Alexander Kolbasov wrote: > > 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/diff/6/?file=1744920#file1744920line2639> > > > > what is 'desc'? Ha, descending. I will use the complete word to avoid confusions. > On June 9, 2017, 6 a.m., Alexander Kolbasov wrote: > > 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/diff/6/?file=1744920#file1744920line2685> > > > > 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? Seems JDO uses lazy fetchs on joins. The authorization maps are not fetched on this first get, only if I attempt to read the authz paths, then they will be read. I confirmed this during debugging too, there are 0 authz paths on this fetch, but if later I want to use a contains() or get() or whatever operation on the Set<>, then JDO will read and populate the Set immediatly. > On June 9, 2017, 6 a.m., Alexander Kolbasov wrote: > > 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/diff/6/?file=1744920#file1744920line2686> > > > > 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. I prefer to use this way to avoid users (and me) to use a assigment = instead of equals ==. Both will work, and we might not find bugs until later. But if you want to be consistent, then that's ok for me. > On June 9, 2017, 6 a.m., Alexander Kolbasov wrote: > > 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/diff/6/?file=1744920#file1744920line2687> > > > > I think IllegalState exception is an unchecked exception - is it what > > we really want here? I think so. This check shouldn't happen unless we are in an scenario when multiple HMSFollower nodes are persisting images, right? I found that IllegalStateException was a perfect match for what I am trying to tell the user. Also, if we are in this weird scenario, then we'll see the logs that something that shouldn't be right happened. > On June 9, 2017, 6 a.m., Alexander Kolbasov wrote: > > 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/diff/6/?file=1744920#file1744920line2695> > > > > What is the official style? Can we write this as > > > > else for ... { > > } > > > > or it isn't kosher? Huh? Is that allowed in Java? I don't think that works. - Sergio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59720/#review177418 ----------------------------------------------------------- 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 > >
