You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2022/07/15 03:04:04 UTC
[GitHub] [hive] nrg4878 commented on a diff in pull request #3429: Hive 26012
nrg4878 commented on code in PR #3429:
URL: https://github.com/apache/hive/pull/3429#discussion_r921752334
##########
metastore_db/README_DO_NOT_TOUCH_FILES.txt:
##########
@@ -0,0 +1,9 @@
+
Review Comment:
remove all files under metastore_db from the PR
##########
metastore_db/service.properties:
##########
@@ -0,0 +1,23 @@
+#/Users/hzhu/upstream/hive/metastore_db
Review Comment:
remove file
##########
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java.orig:
##########
@@ -0,0 +1,6511 @@
+/*
Review Comment:
remove file
##########
standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift.orig:
##########
@@ -0,0 +1,3158 @@
+#!/usr/local/bin/thrift -java
Review Comment:
delete file
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java.orig:
##########
@@ -0,0 +1,4321 @@
+/*
Review Comment:
remove file
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -1360,6 +1367,200 @@ private void create_database_core(RawStore ms, final Database db)
}
}
}
+ */
+
+ private void create_database_core(RawStore ms, final Database db, boolean skipFSwrites)
Review Comment:
it will be better to copy this code in the original method. It is hard for reviewers to understand what changed if you comment out the original code and add a copy of the method. This looks like this code is all new. Hard to compare and understand what changed.
##########
standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift:
##########
@@ -901,7 +901,8 @@ struct AddPartitionsRequest {
4: required bool ifNotExists,
5: optional bool needResult=true,
6: optional string catName,
- 7: optional string validWriteIdList
+ 7: optional string validWriteIdList,
+ 8: optional bool skipFSWrites=false
Review Comment:
please add comment on what this property does.
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClientPreCatalog.java.orig:
##########
@@ -0,0 +1,4027 @@
+/*
Review Comment:
delete file
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -2361,32 +2612,36 @@ private void create_table_core(final RawStore ms, final CreateTableRequest req)
firePreEvent(new PreCreateTableEvent(tbl, db, this));
- if (!TableType.VIRTUAL_VIEW.toString().equals(tbl.getTableType())) {
- if (tbl.getSd().getLocation() == null
- || tbl.getSd().getLocation().isEmpty()) {
- tblPath = wh.getDefaultTablePath(db, tbl.getTableName() + getTableSuffix(tbl), isExternal(tbl));
- } else {
- if (!isExternal(tbl) && !MetaStoreUtils.isNonNativeTable(tbl)) {
- LOG.warn("Location: " + tbl.getSd().getLocation()
- + " specified for non-external table:" + tbl.getTableName());
- }
- tblPath = wh.getDnsPath(new Path(tbl.getSd().getLocation()));
- // ignore suffix if it's already there (direct-write CTAS)
- if (!tblPath.getName().matches("(.*)" + SOFT_DELETE_TABLE_PATTERN)) {
- tblPath = new Path(tblPath + getTableSuffix(tbl));
+ if (!skipFSWrites) {
+ if (!TableType.VIRTUAL_VIEW.toString().equals(tbl.getTableType())) {
+ if (tbl.getSd().getLocation() == null
+ || tbl.getSd().getLocation().isEmpty()) {
+ tblPath = wh.getDefaultTablePath(db, tbl.getTableName() + getTableSuffix(tbl), isExternal(tbl));
+ } else {
+ if (!isExternal(tbl) && !MetaStoreUtils.isNonNativeTable(tbl)) {
+ LOG.warn("Location: " + tbl.getSd().getLocation()
+ + " specified for non-external table:" + tbl.getTableName());
+ }
+ tblPath = wh.getDnsPath(new Path(tbl.getSd().getLocation()));
+ // ignore suffix if it's already there (direct-write CTAS)
+ if (!tblPath.getName().matches("(.*)" + SOFT_DELETE_TABLE_PATTERN)) {
+ tblPath = new Path(tblPath + getTableSuffix(tbl));
+ }
}
+ tbl.getSd().setLocation(tblPath.toString());
}
- tbl.getSd().setLocation(tblPath.toString());
- }
- if (tblPath != null) {
- if (!wh.isDir(tblPath)) {
- if (!wh.mkdirs(tblPath)) {
- throw new MetaException(tblPath
- + " is not a directory or unable to create one");
+ if (tblPath != null) {
+ if (!wh.isDir(tblPath)) {
+ if (!wh.mkdirs(tblPath)) {
+ throw new MetaException(tblPath
+ + " is not a directory or unable to create one");
+ }
+ madeDir = true;
}
- madeDir = true;
}
+ } else {
+ LOG.warn("Skipping the creation of directories for tables.");
Review Comment:
Improve message to indicate that the skipFSWrites was true
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -4045,7 +4304,7 @@ public Partition append_partition_with_environment_context(final String dbName,
Partition ret = null;
Exception ex = null;
try {
- ret = append_partition_common(getMS(), parsedDbName[CAT_NAME], parsedDbName[DB_NAME], tableName, part_vals, envContext);
+ ret = append_partition_common(getMS(), parsedDbName[CAT_NAME], parsedDbName[DB_NAME], tableName, part_vals, false, envContext);
Review Comment:
this will change depending on whether we are making the boolean argument the last
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -3911,7 +4166,7 @@ public List<String> get_table_names_by_filter(
}
private Partition append_partition_common(RawStore ms, String catName, String dbName,
- String tableName, List<String> part_vals,
+ String tableName, List<String> part_vals, boolean skipFSWrites,
Review Comment:
typically, we add new arguments to the end of the current list. This is a private method, so not a big different, but would be nice to make the new argument the last one.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -1174,6 +1180,7 @@ static boolean isDbReplicationTarget(Database db) {
}
// Assumes that the catalog has already been set.
+ /*
Review Comment:
commented code should be deleted. Please clean up this code
##########
standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift:
##########
@@ -2040,7 +2042,9 @@ struct CreateDatabaseRequest {
9: optional i32 createTime,
10: optional string managedLocationUri,
11: optional string type,
- 12: optional string dataConnectorName
+ 12: optional string dataConnectorName,
+ 13: optional Database database,
Review Comment:
not sure I understand the purpose of passing in a Database object into the CreateDatabaseRequest. What is the need for this?
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java.orig:
##########
@@ -0,0 +1,5085 @@
+/*
Review Comment:
remove file
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -1399,6 +1600,55 @@ public void create_database(final Database db)
}
}
+ public void create_database_req(final CreateDatabaseRequest req)
+ throws AlreadyExistsException, InvalidObjectException, MetaException {
+ Database db = req.getDatabase();
+ boolean skipFSWrites = req.isSkipFSWrites();
+ startFunction("create_database", ": " + db.toString());
+ boolean success = false;
+ Exception ex = null;
+ if (!db.isSetCatalogName()) {
+ db.setCatalogName(getDefaultCatalog(conf));
+ }
+ try {
+ try {
+ if (null != get_database_core(db.getCatalogName(), db.getName())) {
+ throw new AlreadyExistsException("Database " + db.getName() + " already exists");
+ }
+ } catch (NoSuchObjectException e) {
+ // expected
+ }
+
+ if (TEST_TIMEOUT_ENABLED) {
Review Comment:
why are we doing this? can you please explain?
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -151,6 +151,12 @@ public class HMSHandler extends FacebookBase implements IHMSHandler {
private IMetaStoreMetadataTransformer transformer;
private static DataConnectorProviderFactory dataconnectorFactory = null;
+ // Used for testing to simulate method timeout.
Review Comment:
what are these for?
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -3961,12 +4216,16 @@ private Partition append_partition_common(RawStore ms, String catName, String db
throw new AlreadyExistsException("Partition already exists:" + part);
}
- if (!wh.isDir(partLocation)) {
- if (!wh.mkdirs(partLocation)) {
- throw new MetaException(partLocation
- + " is not a directory or unable to create one");
+ if (!skipFSWrites) {
+ if (!wh.isDir(partLocation)) {
+ if (!wh.mkdirs(partLocation)) {
+ throw new MetaException(partLocation
+ + " is not a directory or unable to create one");
+ }
+ madeDir = true;
}
- madeDir = true;
+ } else {
+ LOG.warn("Skip creating directories for partitions.");
Review Comment:
Improve log message. Not clear why it is being skipped.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -1399,6 +1600,55 @@ public void create_database(final Database db)
}
}
+ public void create_database_req(final CreateDatabaseRequest req)
+ throws AlreadyExistsException, InvalidObjectException, MetaException {
+ Database db = req.getDatabase();
+ boolean skipFSWrites = req.isSkipFSWrites();
+ startFunction("create_database", ": " + db.toString());
Review Comment:
this should be create_database_req
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -2361,32 +2612,36 @@ private void create_table_core(final RawStore ms, final CreateTableRequest req)
firePreEvent(new PreCreateTableEvent(tbl, db, this));
- if (!TableType.VIRTUAL_VIEW.toString().equals(tbl.getTableType())) {
- if (tbl.getSd().getLocation() == null
- || tbl.getSd().getLocation().isEmpty()) {
- tblPath = wh.getDefaultTablePath(db, tbl.getTableName() + getTableSuffix(tbl), isExternal(tbl));
- } else {
- if (!isExternal(tbl) && !MetaStoreUtils.isNonNativeTable(tbl)) {
- LOG.warn("Location: " + tbl.getSd().getLocation()
- + " specified for non-external table:" + tbl.getTableName());
- }
- tblPath = wh.getDnsPath(new Path(tbl.getSd().getLocation()));
- // ignore suffix if it's already there (direct-write CTAS)
- if (!tblPath.getName().matches("(.*)" + SOFT_DELETE_TABLE_PATTERN)) {
- tblPath = new Path(tblPath + getTableSuffix(tbl));
+ if (!skipFSWrites) {
+ if (!TableType.VIRTUAL_VIEW.toString().equals(tbl.getTableType())) {
+ if (tbl.getSd().getLocation() == null
+ || tbl.getSd().getLocation().isEmpty()) {
+ tblPath = wh.getDefaultTablePath(db, tbl.getTableName() + getTableSuffix(tbl), isExternal(tbl));
+ } else {
+ if (!isExternal(tbl) && !MetaStoreUtils.isNonNativeTable(tbl)) {
+ LOG.warn("Location: " + tbl.getSd().getLocation()
+ + " specified for non-external table:" + tbl.getTableName());
+ }
+ tblPath = wh.getDnsPath(new Path(tbl.getSd().getLocation()));
+ // ignore suffix if it's already there (direct-write CTAS)
+ if (!tblPath.getName().matches("(.*)" + SOFT_DELETE_TABLE_PATTERN)) {
+ tblPath = new Path(tblPath + getTableSuffix(tbl));
+ }
}
+ tbl.getSd().setLocation(tblPath.toString());
}
- tbl.getSd().setLocation(tblPath.toString());
- }
- if (tblPath != null) {
- if (!wh.isDir(tblPath)) {
- if (!wh.mkdirs(tblPath)) {
- throw new MetaException(tblPath
- + " is not a directory or unable to create one");
+ if (tblPath != null) {
Review Comment:
I believe this is the code that should be within the if/else clause check on skipFSWrites not the code above.
##########
standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift:
##########
@@ -2025,7 +2026,8 @@ struct CreateTableRequest {
7: optional list<SQLDefaultConstraint> defaultConstraints,
8: optional list<SQLCheckConstraint> checkConstraints,
9: optional list<string> processorCapabilities,
- 10: optional string processorIdentifier
+ 10: optional string processorIdentifier,
+ 11: optional bool skipFSWrites=false
Review Comment:
add comment about what this new property does
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -1399,6 +1600,55 @@ public void create_database(final Database db)
}
}
+ public void create_database_req(final CreateDatabaseRequest req)
+ throws AlreadyExistsException, InvalidObjectException, MetaException {
+ Database db = req.getDatabase();
+ boolean skipFSWrites = req.isSkipFSWrites();
+ startFunction("create_database", ": " + db.toString());
+ boolean success = false;
+ Exception ex = null;
+ if (!db.isSetCatalogName()) {
+ db.setCatalogName(getDefaultCatalog(conf));
+ }
+ try {
+ try {
+ if (null != get_database_core(db.getCatalogName(), db.getName())) {
+ throw new AlreadyExistsException("Database " + db.getName() + " already exists");
+ }
+ } catch (NoSuchObjectException e) {
+ // expected
+ }
+
+ if (TEST_TIMEOUT_ENABLED) {
+ try {
+ Thread.sleep(TEST_TIMEOUT_VALUE);
+ } catch (InterruptedException e) {
+ // do nothing
+ }
+ Deadline.checkTimeout();
+ }
+ // create database both in metastore and file system or
+ // create database just in metastore
+ // 2 cases: skipFSWrites is false or true
+ create_database_core(getMS(), db, skipFSWrites);
+
Review Comment:
nit: delete empty line
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -2361,32 +2612,36 @@ private void create_table_core(final RawStore ms, final CreateTableRequest req)
firePreEvent(new PreCreateTableEvent(tbl, db, this));
- if (!TableType.VIRTUAL_VIEW.toString().equals(tbl.getTableType())) {
- if (tbl.getSd().getLocation() == null
- || tbl.getSd().getLocation().isEmpty()) {
- tblPath = wh.getDefaultTablePath(db, tbl.getTableName() + getTableSuffix(tbl), isExternal(tbl));
- } else {
- if (!isExternal(tbl) && !MetaStoreUtils.isNonNativeTable(tbl)) {
- LOG.warn("Location: " + tbl.getSd().getLocation()
- + " specified for non-external table:" + tbl.getTableName());
- }
- tblPath = wh.getDnsPath(new Path(tbl.getSd().getLocation()));
- // ignore suffix if it's already there (direct-write CTAS)
- if (!tblPath.getName().matches("(.*)" + SOFT_DELETE_TABLE_PATTERN)) {
- tblPath = new Path(tblPath + getTableSuffix(tbl));
+ if (!skipFSWrites) {
+ if (!TableType.VIRTUAL_VIEW.toString().equals(tbl.getTableType())) {
+ if (tbl.getSd().getLocation() == null
+ || tbl.getSd().getLocation().isEmpty()) {
+ tblPath = wh.getDefaultTablePath(db, tbl.getTableName() + getTableSuffix(tbl), isExternal(tbl));
+ } else {
+ if (!isExternal(tbl) && !MetaStoreUtils.isNonNativeTable(tbl)) {
+ LOG.warn("Location: " + tbl.getSd().getLocation()
+ + " specified for non-external table:" + tbl.getTableName());
+ }
+ tblPath = wh.getDnsPath(new Path(tbl.getSd().getLocation()));
+ // ignore suffix if it's already there (direct-write CTAS)
+ if (!tblPath.getName().matches("(.*)" + SOFT_DELETE_TABLE_PATTERN)) {
+ tblPath = new Path(tblPath + getTableSuffix(tbl));
+ }
}
+ tbl.getSd().setLocation(tblPath.toString());
}
- tbl.getSd().setLocation(tblPath.toString());
Review Comment:
it appears that when skipFSWrites is true, that the tblPath is never calculated. If this is not computed, it will never be set on the table. So how does the table have a location set?
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -6583,7 +6842,7 @@ public Partition append_partition_by_name_with_environment_context(final String
try {
RawStore ms = getMS();
List<String> partVals = getPartValsFromName(ms, parsedDbName[CAT_NAME], parsedDbName[DB_NAME], tbl_name, part_name);
- ret = append_partition_common(ms, parsedDbName[CAT_NAME], parsedDbName[DB_NAME], tbl_name, partVals, env_context);
+ ret = append_partition_common(ms, parsedDbName[CAT_NAME], parsedDbName[DB_NAME], tbl_name, partVals, false, env_context);
Review Comment:
ditto as above
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -1399,6 +1600,55 @@ public void create_database(final Database db)
}
}
+ public void create_database_req(final CreateDatabaseRequest req)
+ throws AlreadyExistsException, InvalidObjectException, MetaException {
+ Database db = req.getDatabase();
+ boolean skipFSWrites = req.isSkipFSWrites();
+ startFunction("create_database", ": " + db.toString());
+ boolean success = false;
+ Exception ex = null;
+ if (!db.isSetCatalogName()) {
+ db.setCatalogName(getDefaultCatalog(conf));
+ }
+ try {
+ try {
+ if (null != get_database_core(db.getCatalogName(), db.getName())) {
+ throw new AlreadyExistsException("Database " + db.getName() + " already exists");
+ }
+ } catch (NoSuchObjectException e) {
+ // expected
+ }
+
+ if (TEST_TIMEOUT_ENABLED) {
+ try {
+ Thread.sleep(TEST_TIMEOUT_VALUE);
+ } catch (InterruptedException e) {
+ // do nothing
+ }
+ Deadline.checkTimeout();
+ }
+ // create database both in metastore and file system or
+ // create database just in metastore
+ // 2 cases: skipFSWrites is false or true
+ create_database_core(getMS(), db, skipFSWrites);
+
+ success = true;
+ } catch (Exception e) {
+ ex = e;
+ if (e instanceof MetaException) {
+ throw (MetaException) e;
+ } else if (e instanceof InvalidObjectException) {
+ throw (InvalidObjectException) e;
+ } else if (e instanceof AlreadyExistsException) {
+ throw (AlreadyExistsException) e;
+ } else {
+ throw newMetaException(e);
+ }
+ } finally {
+ endFunction("create_database", success, ex);
Review Comment:
this should be "create_database_req" I think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org