> On Oct. 28, 2014, 6:20 p.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, > > line 609 > > <https://reviews.apache.org/r/27290/diff/1/?file=735499#file735499line609> > > > > I could see how this could grow in the future, consider splitting this > > out to its own function (with a comment on the purpose) so you just have: > > > > if (isBannedConfigVal(attr)) { > > throw ... > > }
I think an entirely new function is a bit of overkill... but I did address this, sort of, by splitting out the regexes into a "requiredPattern" and an "excludePattern" String. Now we just issue two Pattern.matches(), and if the excludePattern grows we just make the string bigger. - Mike ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27290/#review58842 ----------------------------------------------------------- On Oct. 28, 2014, 5:35 p.m., Mike Yoder wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27290/ > ----------------------------------------------------------- > > (Updated Oct. 28, 2014, 5:35 p.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > SENTRY-507 adding two more items to banned config vals in getConfigVal() > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 6de9992556d176aec546b93d928e33e8aa2f2ab4 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > 23bd7653c94f6337b5cbbc927b39fc811f275482 > > Diff: https://reviews.apache.org/r/27290/diff/ > > > Testing > ------- > > Unit testing; added new tests for additional items. > > > Thanks, > > Mike Yoder > >
