> On June 3, 2017, 1:15 a.m., Alexander Kolbasov wrote: > > This looks good. The important question is whether it will work correctly > > if two instances of Sentry Server are trying to create a full snapshot or > > one of the tries to create a full snapshot while another one updates an > > existing snapshot.
The new patch will prevent multiple instances or transactions to persist new snapshots. > On June 3, 2017, 1:15 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Line 2524 (original), 2531 (patched) > > <https://reviews.apache.org/r/59720/diff/2/?file=1739109#file1739109line2531> > > > > This is existing code, but we can optimize it a bit - we know the exact > > size of the map after query.execute() - we can create the map with a known > > size. That sounds good. > On June 3, 2017, 1:15 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 2551 (patched) > > <https://reviews.apache.org/r/59720/diff/2/?file=1739109#file1739109line2551> > > > > We now have a few functions that get max of some field - is it possible > > to write some generic function to do it? Done. > On June 3, 2017, 1:15 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 2555 (patched) > > <https://reviews.apache.org/r/59720/diff/2/?file=1739109#file1739109line2555> > > > > We may close the query after execute() I tried this, but there are errors when closing the query. Also, I don't see other methods are closing the query either. Is this the correct behavior? > On June 3, 2017, 1:15 a.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 2571 (patched) > > <https://reviews.apache.org/r/59720/diff/2/?file=1739109#file1739109line2571> > > > > Suppose we have another instance that tries to do the same - do we have > > some protection that both of them wouldn't create the same full snapshot? Added a protection at the table level by using a unique boolean value set to TRUE everytime an image is persisted. Also, the image ID will be generated by JDO now instead of manually incrementing the value. This way we store the image + all paths in one single transaction. - Sergio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59720/#review176865 ----------------------------------------------------------- On June 1, 2017, 10:22 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59720/ > ----------------------------------------------------------- > > (Updated June 1, 2017, 10:22 p.m.) > > > Review request for sentry and Alexander Kolbasov. > > > 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/package.jdo > 96ab462d22afaf0f3004926f2855a8bb8cabc53b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > cb05a84cb88151156893f0f9d1885bd83cbc2abb > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > c5dddfb07bdfcb4c2bfee51f5cd7c684eff65a80 > > > Diff: https://reviews.apache.org/r/59720/diff/2/ > > > Testing > ------- > > All unit tests run locally, but there are some flaky tests on Jenkins no > related to this patch. > > > Thanks, > > Sergio Pena > >
