> On June 15, 2017, 5 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MHiveMetastoreImage.java > > Lines 28 (patched) > > <https://reviews.apache.org/r/59720/diff/8/?file=1751335#file1751335line28> > > > > Naming - although the info comes from HMS, it isn't any image of > > HiveMetastore. It is essentially the generation.
I couldn't find a better name before, but now I did. What about MAuthzPathsSnapshotId? This reflects the intention of the table that stores only snapshot identifiers for authz-paths. > On June 15, 2017, 5 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 2582 (patched) > > <https://reviews.apache.org/r/59720/diff/8/?file=1751337#file1751337line2587> > > > > Do we really need to do it within the same transaction? It could be Ok, > > but if this is really expensive it might be better to do it asynchronously. > > We can use this approach and see whether it causes problems. This is tricky. There are some scenarios that I think it may cause conflicts. 1. Deleting all in one transaction, then persisting in another transaction may cause that the HMSFollower detects that MAuthzPathsMapping is empty and attempts to persist a new image while one is in progress. 2. If a HMSFollower detected that MAuthzPathsMapping is empty and it triggers a new transaction to persist a snapshot, but just milliseconds before, the old thread finishes persisting, then this new thread will persist a new image (with a new ID) but containg duplicated content on MPaths. There are some methods that get information about MPaths without joining to MAuthzPathsMapping, so this will get incorrect results. So I thought having this delete in the same transaction would help me avoiding the above issues. > On June 15, 2017, 5 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Line 2583 (original), 2613 (patched) > > <https://reviews.apache.org/r/59720/diff/8/?file=1751337#file1751337line2618> > > > > Can this ever happen? This should not happen ever; but the backend DB does not have any constraint that a user modifies the table manually. Should we at least display an error in case this is found for some weird reason? - Sergio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59720/#review177967 ----------------------------------------------------------- On June 14, 2017, 6:25 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59720/ > ----------------------------------------------------------- > > (Updated June 14, 2017, 6:25 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 > 8b19c88e52400ce096cf8f38fe2ca0313f7e46e1 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 29878c5f1213062cb9d0a1f9666a19004b67dc52 > > > Diff: https://reviews.apache.org/r/59720/diff/8/ > > > Testing > ------- > > All unit tests run locally, but there are some flaky tests on Jenkins no > related to this patch. > > > Thanks, > > Sergio Pena > >
