> 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? > > Sergio Pena wrote: > 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.
I think in other part of the code we use something like "this.id == expectedId" - which is at least more readable. Are there any reasons not to use it? At least, it is worth a comment explaining why it works. > 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. > > Sergio Pena wrote: > 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. We may have a subtle problem if we only preserve a single row. Suppose we have two threads A and B. A reads the ID value (let's say 10) and B reads the same value. Now A updates it to 11 and then to 12. B updates to 11 and writes. 11 != 12 so write is successfull. As a result A's update to 11 and 12 is lost. This isn't a very likely scenario here since we are talking about full HMS snapshots, so I don't think it is a realistic issue, but still something to keep in mind. > 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 < > > Sergio Pena wrote: > I don't understand. This doc is already there. Yeah, this is a problem with the original doc - formatting is wrong. I don't know what was the original intention, but looks like these < should be just removed. > 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. > > Sergio Pena wrote: > 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. Can you add it with the code that actually uses it? > 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? > > Sergio Pena wrote: > 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. Please add comment explaining this - it is non-trivial. And some innocent change like a new log statement may suddenly trigger a fetch, so it is worh adding a warning. > 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? > > Sergio Pena wrote: > 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. The problem is that IllegalStateException is unchecked - it is for things that should never ever happen and there is no way to get around the situation. Having multiple HMSFollower nodes persisting the image is rather unlikely but may happen once in a while and it isn't the end of the world. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59720/#review177418 ----------------------------------------------------------- On June 9, 2017, 4:17 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59720/ > ----------------------------------------------------------- > > (Updated June 9, 2017, 4:17 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 > 17856a4425be5c8971c51f7cfd9b2ef06001a2ab > > 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/7/ > > > Testing > ------- > > All unit tests run locally, but there are some flaky tests on Jenkins no > related to this patch. > > > Thanks, > > Sergio Pena > >
