----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42867/#review116709 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java (line 116) <https://reviews.apache.org/r/42867/#comment177781> I don't think we want to add the test specific code into the product side. What I suggest is to make this testable by adding a new property to PathUpdates: // Visible for testing setConfiguration(Configuration config) { this.CONF = config; } then in the *test* file, just do: // Only update the config if running in local mode. If tests run against a real cluster the default configuration should be used bool isExternalSentry = new Boolean(System.getProperty(EXTERNAL_SENTRY_SERVICE, "false")); if (!isExternalSentry) { Configuration config = new Config(); config.setDefaultURI() PathUpdates.setConfiguration(config); } Note that you need to consider the same test case running in the local dev environment (we need to use the test hook) vs the same test running on a cluster. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java (line 200) <https://reviews.apache.org/r/42867/#comment177782> I don't think we need to introduce a new property, you should use the existing System.getProperty(EXTERNAL_SENTRY_SERVICE, "false")). We don't want to always set the config, only when the tests are running in local mode. Looks good, just a couple of comments around how the test hook is being used. - Lenni Kuff On Jan. 28, 2016, 2:21 a.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42867/ > ----------------------------------------------------------- > > (Updated Jan. 28, 2016, 2:21 a.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > Change-Id: I9976086dbdf3c57c931d729870eeeaa6f7e6fa29 > There a few edge cases where Paths update may be passed a path which lacks a > URI Scheme. > i.e /user/hive/warehouse > In this situation PathsUpdate will throw an uncaught NPE at the Precondition > check. > Use the default scheme of the filesystem and silently ignore other schemeless > paths. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > 1dcb75a3dc7735e11289ab75424199ab1a086ce2 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java > fc7f3245a274a60d811fdbbe10119ad54fa687cd > > Diff: https://reviews.apache.org/r/42867/diff/ > > > Testing > ------- > > Test done in TestHDFSIntegration.testMissingScheme. > > > Thanks, > > Hao Hao > >
