> On Sept. 20, 2013, 11:26 p.m., Shreepadma Venugopalan wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java, > > line 166 > > <https://reviews.apache.org/r/14165/diff/1/?file=352568#file352568line166> > > > > Shouldn't deprecatedConfigs be a map of the deprecated config's name to > > the one that' currently in use? > > Gregory Chanan wrote: > It depends on how you want to use it, I guess. Here's my thinking: > > 1) the code should work if the deprecated properties are provided, as > long as the new property names aren't also provided > 2) the binding code will only call the new property names. That is, > hive-binding will only call get(AUTHZ_PROVIDER) not > get(AUTHZ_PROVIDER_DEPRECATED) > 3) given #1 and #2, a call to get(AUTHZ_PROVIDER) needs to return the > same value as get(AUTHZ_PROVIDER_DEPRECATED) if AUTHZ_PROVIDER is not > specified > 4) The most straightforward way of accomplishing #3 is to have a map from > non-deprecated-name to deprecated-name, e.g. AUTHZ_PROVIDER -> > AUTH_PROVIDER_DEPRECATED. > > Make sense? We can rename the collection if you think that improves > things.
It wasn't clear from the code what kind of mapping was needed, unfortunately this piece of code is confusing. This code looks good to me. Can you rename the collection and add comments, so that people reading the code don't have the same questions? > On Sept. 20, 2013, 11:26 p.m., Shreepadma Venugopalan wrote: > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java, > > line 79 > > <https://reviews.apache.org/r/14165/diff/1/?file=352569#file352569line79> > > > > I don't see sentry-deprecated-site.xml in this patch. Does this patch > > assume SENTRY-16 is available? > > Gregory Chanan wrote: > Yes, my mistake. I'll rebase now that SENTRY-16 has been committed. Thanks. I'd like to look at the updated review. - Shreepadma ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14165/#review26307 ----------------------------------------------------------- On Sept. 17, 2013, 6:27 p.m., Gregory Chanan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14165/ > ----------------------------------------------------------- > > (Updated Sept. 17, 2013, 6:27 p.m.) > > > Review request for sentry and Shreepadma Venugopalan. > > > Bugs: SENTRY-4 > https://issues.apache.org/jira/browse/SENTRY-4 > > > Repository: sentry > > > Description > ------- > > SENTRY-4: Rename Configuration properties that mention hive but are sentry > related > > As discussed in the JIRA, does the following: > 0) add new sentry.* properties, as discussed in SENTRY-14 > 1) remove deprecation code around hive.access.* properties > 2) add deprecation code around hive.sentry.* properties > 3) warn about deprecated configs only if they actually end up being used > > I'm a bit unsure of spacing (how many spaces is a tab? I used two in most > places -- perhaps something to add to the how to contribute page?) > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java > 5190ba6 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java > bfd58fa > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java > 20d4e8f > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzConf.java > de59546 > sentry-binding/sentry-binding-hive/src/test/resources/access-site.xml > 1936c21 > sentry-binding/sentry-binding-hive/src/test/resources/sentry-site.xml > beef40d > sentry-tests/src/test/resources/access-site.xml e4de05a > sentry-tests/src/test/resources/sentry-site.xml de0c9cf > > Diff: https://reviews.apache.org/r/14165/diff/ > > > Testing > ------- > > Ran the unit tests and they were successful: > mvn test -Pdownload-hadoop > > > Thanks, > > Gregory Chanan > >
