> On June 27, 2018, 3:53 p.m., Peter Vary wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/FileUtils.java > > Lines 526 (patched) > > <https://reviews.apache.org/r/67753/diff/2/?file=2046288#file2046288line526> > > > > Are we sure that the location is not null at this stage? > > Oleksiy Sayankin wrote: > Saying _location_, do you mean variable _path_ here or the result of > _getFileSystem(conf)_? > > Peter Vary wrote: > Path... sorry for the confusion! > > Oleksiy Sayankin wrote: > Yes. From sources: > > Path dbPath = wh.determineDatabasePath(cat, db); > String dbLocationUri = wh.getDnsPath(dbPath).toString(); > Configuration conf = getConf(); > if (!MetastoreConf.getBoolVar(conf, > ConfVars.METASTORE_ALLOW_NEW_DB_IN_EXISTING_DIRECTORY) && FileUtils > .exists(dbPath, conf)) { > > where method _determineDatabasePath()_ is: > > > public Path determineDatabasePath(Catalog cat, Database db) throws > MetaException { > if (db.isSetLocationUri()) { > return getDnsPath(new Path(db.getLocationUri())); > } > if (cat == null || > cat.getName().equalsIgnoreCase(DEFAULT_CATALOG_NAME)) { > if (db.getName().equalsIgnoreCase(DEFAULT_DATABASE_NAME)) { > return getWhRoot(); > } else { > return new Path(getWhRoot(), dbDirFromDbName(db)); > } > } else { > return new Path(getDnsPath(new Path(cat.getLocationUri())), > dbDirFromDbName(db)); > } > } > > look at each __return__ statement. Every time we use __new Path()__ > directly or as an argument to a method _getDnsPath()_. And it also uses > __return new Path()__. See > > public static Path getDnsPath(Path path, Configuration conf) throws > MetaException { > FileSystem fs = getFs(path, conf); > return (new Path(fs.getUri().getScheme(), fs.getUri().getAuthority(), > path > .toUri().getPath())); > } > > So _dbPath_ can't be __null__ here. Hence > > return path.getFileSystem(conf).exists(path); > > works fine and there is no NPE. > > Peter Vary wrote: > Thanks for the explanation! > > Cool, then add some comment javadoc for the new public method > (FileUtils.exists), that the path should not be null, and I think we should > "only" wait for the precommit/yetus test results :)
Done - Oleksiy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67753/#review205424 ----------------------------------------------------------- On June 27, 2018, 5:02 p.m., Oleksiy Sayankin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67753/ > ----------------------------------------------------------- > > (Updated June 27, 2018, 5:02 p.m.) > > > Review request for hive, Zoltan Haindrich and Zoltan Haindrich. > > > Repository: hive-git > > > Description > ------- > > Added hive.metastore.allow.new.db.in.existing.directory property > > > Diffs > ----- > > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > e9d7e7c397 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java > f3b909ca44 > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/FileUtils.java > 963e12f9d8 > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/TestMetaCreateObjects.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/67753/diff/5/ > > > Testing > ------- > > > Thanks, > > Oleksiy Sayankin > >
