> On April 21, 2015, 5:47 a.m., Prasad Mujumdar wrote: > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/HadoopGroupResourceAuthorizationProvider.java, > > line 36 > > <https://reviews.apache.org/r/32847/diff/1/?file=915089#file915089line36> > > > > This will make Sentry to intantiate a new group object instead of > > using a static cached instance. My concern is the increased network and > > session requirement for non-default group mappings like Hadoop's > > LDAPGroupMapping. With this patch, we'll keep opening new connection to > > LDAP server every time. > > > > How about caching that group object in > > HadoopGroupResourceAuthorizationProvider instead of creating a new one > > every time ?
I'm not sure I understand the use case. With solr, we only create the authorization provider once. Is that not the case with the service? What exactly would you propose caching? The group object is dependent on the configuration. Just caching one group object necessarily ignores the Configuration and gives you the same broken interface as the Hadoop groups; it creates the groups once, based on the first configuration passed in, and silently ignores subsequent attempts to have groups based on different Configurations. Or would we have a hashmap of Configuration or Name to group mappings? - Gregory ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32847/#review80912 ----------------------------------------------------------- On April 4, 2015, 1:11 a.m., Gregory Chanan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32847/ > ----------------------------------------------------------- > > (Updated April 4, 2015, 1:11 a.m.) > > > Review request for sentry, shen guoquan, Prasad Mujumdar, and Vamsee > Yarlagadda. > > > Repository: sentry > > > Description > ------- > > Adds a unit test for using different group mappings that fails without the > patch and passes with the patch. > > The basic issue is that in solr we don't have hadoop Configuration files on > the classpath and the hadoop group inevitably get set to "new > Configuration()" (the jira has an example where calling FileSystem.get(conf) > sets up the group mappings to "new Configuration" -- not the conf you pass > in. But this is just one example, I've seen it all over the place in hadoop > code. > > So, this patch does two things: > 1) sets the configuration earlier in the binding so we get the group mappings > before the provider backend constructor (which reads from hdfs, so can call > FileSystem.get(...) > 2) changes the HadoopGroupResourceAuthorizationProvider from using > Groups.getUserToGroupsMappingService(...) to using new Groups(...). That is, > the groups now use the actual configuration you passed in rather than > whatever Groups were originally set up as (possibly mistakenly, as in the > example above). In the case where Groups.getUserToGroupMappingService(...) > hasn't been called yet, the behavior is the same. This is definitely an > improvement in Solr, and I don't think it should affect hive because from my > understanding Hive has the configuration files on the classpath, so the > behavior should be identical. It would be good if someone could confirm that > though. > > > Diffs > ----- > > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java > 373ee8c7fd0b3bc569de39dd664c8876d5597b30 > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/binding/solr/TestSolrAuthzBinding.java > 1bc01a2d837f0afc78547c1667f701cdbd1f6193 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/HadoopGroupMappingService.java > 14e2d05c919ec540acf845056ed8972239a27768 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/HadoopGroupResourceAuthorizationProvider.java > 626fd909cac9630dc11a2386886423bde4fba190 > > Diff: https://reviews.apache.org/r/32847/diff/ > > > Testing > ------- > > Ran the changed unit test. We'll see what the QA bot says. > > > Thanks, > > Gregory Chanan > >
