Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra merged PR #10001: URL: https://github.com/apache/iceberg/pull/10001 -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1537915830
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##
@@ -304,23 +310,13 @@ protected void doCommit(TableMetadata base, TableMetadata
metadata) {
throw new CommitFailedException(e);
} finally {
- cleanupMetadataAndUnlock(commitStatus, newMetadataLocation, lock);
+ HiveOperationsBase.cleanupMetadataAndUnlock(io(), commitStatus,
newMetadataLocation, lock);
Review Comment:
It expects interface name with static method . Plz let me know if you think
other way around.
https://github.com/apache/iceberg/assets/4146188/9d1f4f78-c607-41f2-9c10-04138c74a566;>
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1537915830
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##
@@ -304,23 +310,13 @@ protected void doCommit(TableMetadata base, TableMetadata
metadata) {
throw new CommitFailedException(e);
} finally {
- cleanupMetadataAndUnlock(commitStatus, newMetadataLocation, lock);
+ HiveOperationsBase.cleanupMetadataAndUnlock(io(), commitStatus,
newMetadataLocation, lock);
Review Comment:
It expects interface with static method . Plz let me know if you think other
way around.
https://github.com/apache/iceberg/assets/4146188/9d1f4f78-c607-41f2-9c10-04138c74a566;>
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1537175702
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##
@@ -304,23 +310,13 @@ protected void doCommit(TableMetadata base, TableMetadata
metadata) {
throw new CommitFailedException(e);
} finally {
- cleanupMetadataAndUnlock(commitStatus, newMetadataLocation, lock);
+ HiveOperationsBase.cleanupMetadataAndUnlock(io(), commitStatus,
newMetadataLocation, lock);
Review Comment:
```suggestion
cleanupMetadataAndUnlock(io(), commitStatus, newMetadataLocation,
lock);
```
I don't think you need to qualify this method
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535437170
##
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##
@@ -1041,7 +1041,7 @@ public void testNotExposeTableProperties() {
.doesNotContainKey(CURRENT_SNAPSHOT_ID)
.doesNotContainKey(CURRENT_SNAPSHOT_TIMESTAMP);
-ops.setSchema(metadata, parameters);
+ops.setSchema(metadata.schema(), parameters);
Review Comment:
no need to change this, as we need to keep the original method signature
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535436900
##
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##
@@ -1041,7 +1041,7 @@ public void testNotExposeTableProperties() {
.doesNotContainKey(CURRENT_SNAPSHOT_ID)
.doesNotContainKey(CURRENT_SNAPSHOT_TIMESTAMP);
-ops.setSchema(metadata, parameters);
+ops.setSchema(metadata.schema(), parameters);
Review Comment:
```suggestion
ops.setSchema(metadata, parameters);
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535422769
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##
@@ -457,6 +448,16 @@ public String table() {
return tableName;
}
+ @Override
+ public String catalogName() {
Review Comment:
please remove these two methods
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001: URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535422346 ## hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java: ## @@ -365,6 +353,10 @@ private void setHmsTableParameters( } // Set the basic statistics +Map summary = Review Comment: why is this change needed? -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001: URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535422346 ## hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java: ## @@ -365,6 +353,10 @@ private void setHmsTableParameters( } // Set the basic statistics +Map summary = Review Comment: why is this change needed? -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535421842
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##
@@ -304,30 +305,16 @@ protected void doCommit(TableMetadata base, TableMetadata
metadata) {
throw new CommitFailedException(e);
} finally {
- cleanupMetadataAndUnlock(commitStatus, newMetadataLocation, lock);
+ HiveOperationsBase.cleanupMetadataAndUnlock(io(), commitStatus,
newMetadataLocation, lock);
}
LOG.info(
"Committed to table {} with the new metadata location {}", fullName,
newMetadataLocation);
}
- @VisibleForTesting
- Table loadHmsTable() throws TException, InterruptedException {
-try {
- return metaClients.run(client -> client.getTable(database, tableName));
-} catch (NoSuchObjectException nte) {
- LOG.trace("Table not found {}", fullName, nte);
- return null;
-}
- }
+ public void setHmsParameters(
Review Comment:
please revert this to how it was before
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535420615
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##
@@ -304,30 +305,16 @@ protected void doCommit(TableMetadata base, TableMetadata
metadata) {
throw new CommitFailedException(e);
} finally {
- cleanupMetadataAndUnlock(commitStatus, newMetadataLocation, lock);
+ HiveOperationsBase.cleanupMetadataAndUnlock(io(), commitStatus,
newMetadataLocation, lock);
}
LOG.info(
"Committed to table {} with the new metadata location {}", fullName,
newMetadataLocation);
}
- @VisibleForTesting
- Table loadHmsTable() throws TException, InterruptedException {
-try {
- return metaClients.run(client -> client.getTable(database, tableName));
-} catch (NoSuchObjectException nte) {
- LOG.trace("Table not found {}", fullName, nte);
- return null;
-}
- }
+ public void setHmsParameters(
+ TableMetadata metadata, Table tbl, String newMetadataLocation,
Set obsoleteProps) {
- private void setHmsTableParameters(
Review Comment:
please revert these changes
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##
@@ -304,30 +305,16 @@ protected void doCommit(TableMetadata base, TableMetadata
metadata) {
throw new CommitFailedException(e);
} finally {
- cleanupMetadataAndUnlock(commitStatus, newMetadataLocation, lock);
+ HiveOperationsBase.cleanupMetadataAndUnlock(io(), commitStatus,
newMetadataLocation, lock);
}
LOG.info(
"Committed to table {} with the new metadata location {}", fullName,
newMetadataLocation);
}
- @VisibleForTesting
- Table loadHmsTable() throws TException, InterruptedException {
-try {
- return metaClients.run(client -> client.getTable(database, tableName));
-} catch (NoSuchObjectException nte) {
- LOG.trace("Table not found {}", fullName, nte);
- return null;
-}
- }
+ public void setHmsParameters(
+ TableMetadata metadata, Table tbl, String newMetadataLocation,
Set obsoleteProps) {
- private void setHmsTableParameters(
Review Comment:
please revert these changes in this method
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535419303
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java:
##
@@ -123,13 +185,14 @@ default void persistTable(Table hmsTable, boolean
updateHiveTable, String metada
}
}
- static StorageDescriptor storageDescriptor(TableMetadata metadata, boolean
hiveEngineEnabled) {
Review Comment:
same as above, we need to keep `StorageDescriptor
storageDescriptor(TableMetadata metadata, boolean hiveEngineEnabled)` but it
can internally call `static StorageDescriptor storageDescriptor(Schema schema,
String location, boolean hiveEngineEnabled)`
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535418537
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java:
##
@@ -76,11 +138,11 @@ default boolean exposeInHmsProperties() {
return maxHiveTablePropertySize() > 0;
}
- default void setSchema(TableMetadata metadata, Map
parameters) {
Review Comment:
we need to keep the original method (for backwards compatibiliy), but the
method can refer to the new `setSchema()` method:
```
default void setSchema(TableMetadata metadata, Map
parameters) {
setSchema(metadata.schema(), parameters);
}
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535416777
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java:
##
@@ -62,6 +82,48 @@ interface HiveOperationsBase {
String table();
+ String catalogName();
+
+ ContentType contentType();
+
+ default Table loadHmsTable() throws TException, InterruptedException {
+try {
+ return metaClients().run(client -> client.getTable(database(), table()));
+} catch (NoSuchObjectException nte) {
+ LOG.trace(
+ "{} not found {}", contentType(), catalogName() + "." + database() +
"." + table(), nte);
+ return null;
+}
+ }
+
+ default void setCommonHmsParameters(
+ Table tbl,
+ String tableTypeProp,
+ String newMetadataLocation,
+ Schema schema,
+ String uuid,
+ Set obsoleteProps,
+ Supplier previousLocationSupplier) {
+Map parameters =
+Optional.ofNullable(tbl.getParameters()).orElseGet(Maps::newHashMap);
+
+if (!obsoleteProps.contains(TableProperties.UUID) && uuid != null) {
+ parameters.put(TableProperties.UUID, uuid);
Review Comment:
In
https://github.com/apache/iceberg/pull/9852/files#diff-db46657b84d66e084e15f31b8dab21577efb2ae7102863f94c6c9477782de676R206
you're passing the View UUID and I wonder whether this is actually correct to
do so?
This is another example where it can be dangerous to have a common method to
set something that seems could be common across tables/views.
I would suggest to remove `setCommonHmsParameters()` from this API
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535406841
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java:
##
@@ -62,6 +82,48 @@ interface HiveOperationsBase {
String table();
+ String catalogName();
Review Comment:
I don't think we should be introducing `catalogName()`/ `contentType()` to
this API just for printing it in a `TRACE` log inside `loadHmsTable()`. Please
remove `catalogName()`/ `contentType()` from this API
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535407624
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java:
##
@@ -62,6 +82,48 @@ interface HiveOperationsBase {
String table();
+ String catalogName();
+
+ ContentType contentType();
+
+ default Table loadHmsTable() throws TException, InterruptedException {
+try {
+ return metaClients().run(client -> client.getTable(database(), table()));
+} catch (NoSuchObjectException nte) {
+ LOG.trace(
+ "{} not found {}", contentType(), catalogName() + "." + database() +
"." + table(), nte);
Review Comment:
```suggestion
"{} not found {}", contentType(), database() + "." + table(), nte);
```
this is just TRACE logging, so it should be ok to not have the catalog. I'm
not even sure that logging stmt should even exist
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535393968
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -273,6 +284,21 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
}
}
+ private void validateTableIsIcebergTableOrView(
+ HiveOperationsBase.ContentType contentType,
+ String catalogName,
+ Table table,
+ TableIdentifier identifier) {
Review Comment:
```suggestion
String fullTableName) {
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535393632
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -273,6 +284,21 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
}
}
+ private void validateTableIsIcebergTableOrView(
+ HiveOperationsBase.ContentType contentType,
+ String catalogName,
Review Comment:
```suggestion
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535393422
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -239,7 +250,7 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
try {
Table table = clients.run(client -> client.getTable(fromDatabase,
fromName));
- HiveOperationsBase.validateTableIsIceberg(table, fullTableName(name,
from));
+ validateTableIsIcebergTableOrView(contentType, name, table, from);
Review Comment:
rather than passing the catalog name / table / from separately, it's
probably better to just pass the table and the fullTableName:
`fullTableName(name, from)`
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535389326
##
core/src/test/java/org/apache/iceberg/TestCatalogUtil.java:
##
@@ -223,6 +223,32 @@ public void loadCustomMetricsReporter_badClass() {
.hasMessageContaining("does not implement MetricsReporter");
}
+ @Test
+ public void fullTableNameWithDifferentValues() {
+String uriTypeCatalogName = "thrift://host:port/db.table";
+String nameSpaceWithOneLevel = "ns";
Review Comment:
```suggestion
String namespace = "ns";
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535389035
##
core/src/test/java/org/apache/iceberg/TestCatalogUtil.java:
##
@@ -223,6 +223,32 @@ public void loadCustomMetricsReporter_badClass() {
.hasMessageContaining("does not implement MetricsReporter");
}
+ @Test
+ public void fullTableNameWithDifferentValues() {
+String uriTypeCatalogName = "thrift://host:port/db.table";
+String nameSpaceWithOneLevel = "ns";
+String nameSpaceWithTwoLevel = "ns.l2";
Review Comment:
```suggestion
String namespaceWithTwoLevels = "ns.l2";
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1535385693
##
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.apache.iceberg.TableProperties.COMMIT_NUM_STATUS_CHECKS;
+import static
org.apache.iceberg.TableProperties.COMMIT_NUM_STATUS_CHECKS_DEFAULT;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS_DEFAULT;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS_DEFAULT;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS_DEFAULT;
+
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+import org.apache.iceberg.util.PropertyUtil;
+import org.apache.iceberg.util.Tasks;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class BaseMetastoreOperations {
Review Comment:
```suggestion
public abstract class BaseMetastoreOperations {
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533819440
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##
@@ -282,7 +281,12 @@ protected void doCommit(TableMetadata base, TableMetadata
metadata) {
database,
tableName,
e);
-commitStatus = checkCommitStatus(newMetadataLocation, metadata);
+commitStatus =
Review Comment:
Here we can use the original method from
`BaseMetastoreTableOperations.checkCommitStatus` too.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533818010
##
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##
@@ -309,65 +304,39 @@ protected enum CommitStatus {
* @return Commit Status of Success, Failure or Unknown
*/
protected CommitStatus checkCommitStatus(String newMetadataLocation,
TableMetadata config) {
-int maxAttempts =
-PropertyUtil.propertyAsInt(
-config.properties(), COMMIT_NUM_STATUS_CHECKS,
COMMIT_NUM_STATUS_CHECKS_DEFAULT);
-long minWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS,
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS_DEFAULT);
-long maxWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS,
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS_DEFAULT);
-long totalRetryMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS,
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS_DEFAULT);
-
-AtomicReference status = new
AtomicReference<>(CommitStatus.UNKNOWN);
-
-Tasks.foreach(newMetadataLocation)
-.retry(maxAttempts)
-.suppressFailureWhenFinished()
-.exponentialBackoff(minWaitMs, maxWaitMs, totalRetryMs, 2.0)
-.onFailure(
-(location, checkException) ->
-LOG.error("Cannot check if commit to {} exists.", tableName(),
checkException))
-.run(
-location -> {
- TableMetadata metadata = refresh();
- String currentMetadataFileLocation =
metadata.metadataFileLocation();
- boolean commitSuccess =
- currentMetadataFileLocation.equals(newMetadataLocation)
- || metadata.previousFiles().stream()
- .anyMatch(log ->
log.file().equals(newMetadataLocation));
- if (commitSuccess) {
-LOG.info(
-"Commit status check: Commit to {} of {} succeeded",
-tableName(),
-newMetadataLocation);
-status.set(CommitStatus.SUCCESS);
- } else {
-LOG.warn(
-"Commit status check: Commit to {} of {} unknown, new
metadata location is not current "
-+ "or in history",
-tableName(),
-newMetadataLocation);
- }
-});
-
-if (status.get() == CommitStatus.UNKNOWN) {
- LOG.error(
- "Cannot determine commit state to {}. Failed during checking {}
times. "
- + "Treating commit state as unknown.",
- tableName(),
- maxAttempts);
+return CommitStatus.valueOf(
+checkCommitStatus(
+tableName(),
+newMetadataLocation,
+config.properties(),
+() -> checkCurrentMetadataLocation(newMetadataLocation))
+.name());
+ }
+
+ /**
+ * Checks the new metadata location presents or not after refreshing the
table.
+ *
+ * @param newMetadataLocation newly written metadata location
+ * @return true if the new metadata location presents with current or
previous metadata files.
+ */
+ protected boolean checkCurrentMetadataLocation(String newMetadataLocation) {
Review Comment:
It depends how the
[checkCommitStatus](https://github.com/apache/iceberg/pull/10001/files#diff-e502621d52f86cf0ec3187dda30ac61f6b76efb7b6276bc8d233ccb2c836fb98R284)
is being called. It it is using
`BaseMetastoreTableOperations.checkCommitStatus` it can be private . it it
is using `BaseMetastoreOperations.checkCommitStatus` it should be protected.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533741453
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##
@@ -304,30 +305,16 @@ protected void doCommit(TableMetadata base, TableMetadata
metadata) {
throw new CommitFailedException(e);
} finally {
- cleanupMetadataAndUnlock(commitStatus, newMetadataLocation, lock);
+ HiveOperationsBase.cleanupMetadataAndUnlock(io(), commitStatus,
newMetadataLocation, lock);
}
LOG.info(
"Committed to table {} with the new metadata location {}", fullName,
newMetadataLocation);
}
- @VisibleForTesting
- Table loadHmsTable() throws TException, InterruptedException {
-try {
- return metaClients.run(client -> client.getTable(database, tableName));
-} catch (NoSuchObjectException nte) {
- LOG.trace("Table not found {}", fullName, nte);
- return null;
-}
- }
+ public void setHmsParameters(
Review Comment:
It has been split into two parts , one is specific to Table like
`snapshot,stats, etc.` Other pat is common parameters as ` schema, location,
uuid. `
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533738195
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##
@@ -240,9 +239,9 @@ protected void doCommit(TableMetadata base, TableMetadata
metadata) {
tbl, updateHiveTable, hiveLockEnabled(metadata, conf) ? null :
baseMetadataLocation);
lock.ensureActive();
-commitStatus = CommitStatus.SUCCESS;
+commitStatus = BaseMetastoreOperations.CommitStatus.SUCCESS;
Review Comment:
Since we are depreciating `BaseMetastoreTableOperations.CommitStatus` .
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533723838
##
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##
@@ -309,65 +304,39 @@ protected enum CommitStatus {
* @return Commit Status of Success, Failure or Unknown
*/
protected CommitStatus checkCommitStatus(String newMetadataLocation,
TableMetadata config) {
-int maxAttempts =
-PropertyUtil.propertyAsInt(
-config.properties(), COMMIT_NUM_STATUS_CHECKS,
COMMIT_NUM_STATUS_CHECKS_DEFAULT);
-long minWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS,
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS_DEFAULT);
-long maxWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS,
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS_DEFAULT);
-long totalRetryMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS,
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS_DEFAULT);
-
-AtomicReference status = new
AtomicReference<>(CommitStatus.UNKNOWN);
-
-Tasks.foreach(newMetadataLocation)
-.retry(maxAttempts)
-.suppressFailureWhenFinished()
-.exponentialBackoff(minWaitMs, maxWaitMs, totalRetryMs, 2.0)
-.onFailure(
-(location, checkException) ->
-LOG.error("Cannot check if commit to {} exists.", tableName(),
checkException))
-.run(
-location -> {
- TableMetadata metadata = refresh();
- String currentMetadataFileLocation =
metadata.metadataFileLocation();
- boolean commitSuccess =
- currentMetadataFileLocation.equals(newMetadataLocation)
- || metadata.previousFiles().stream()
- .anyMatch(log ->
log.file().equals(newMetadataLocation));
- if (commitSuccess) {
-LOG.info(
-"Commit status check: Commit to {} of {} succeeded",
-tableName(),
-newMetadataLocation);
-status.set(CommitStatus.SUCCESS);
- } else {
-LOG.warn(
-"Commit status check: Commit to {} of {} unknown, new
metadata location is not current "
-+ "or in history",
-tableName(),
-newMetadataLocation);
- }
-});
-
-if (status.get() == CommitStatus.UNKNOWN) {
- LOG.error(
- "Cannot determine commit state to {}. Failed during checking {}
times. "
- + "Treating commit state as unknown.",
- tableName(),
- maxAttempts);
+return CommitStatus.valueOf(
+checkCommitStatus(
+tableName(),
+newMetadataLocation,
+config.properties(),
+() -> checkCurrentMetadataLocation(newMetadataLocation))
+.name());
+ }
+
+ /**
+ * Checks the new metadata location presents or not after refreshing the
table.
+ *
+ * @param newMetadataLocation newly written metadata location
+ * @return true if the new metadata location presents with current or
previous metadata files.
+ */
+ protected boolean checkCurrentMetadataLocation(String newMetadataLocation) {
Review Comment:
I think this should be `private`. Why would it need to be available for
subclasses? I don't think subclasses would call this code?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533719987
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -222,9 +211,29 @@ public boolean dropTable(TableIdentifier identifier,
boolean purge) {
@Override
public void renameTable(TableIdentifier from, TableIdentifier originalTo) {
-if (!isValidIdentifier(from)) {
- throw new NoSuchTableException("Invalid identifier: %s", from);
-}
+renameContent(from, originalTo, HiveOperationsBase.ContentType.TABLE);
+ }
+
+ private List listIcebergTables(
+ List tableNames, Namespace namespace, String tableTypeProp)
+ throws TException, InterruptedException {
+List tableObjects =
+clients.run(client -> client.getTableObjectsByName(namespace.level(0),
tableNames));
+return tableObjects.stream()
+.filter(
+table ->
+table.getParameters() != null
+&& tableTypeProp.equalsIgnoreCase(
+
table.getParameters().get(BaseMetastoreTableOperations.TABLE_TYPE_PROP)))
+.map(table -> TableIdentifier.of(namespace, table.getTableName()))
+.collect(Collectors.toList());
+ }
+
+ private void renameContent(
+ TableIdentifier from,
+ TableIdentifier originalTo,
+ HiveOperationsBase.ContentType contentType) {
+Preconditions.checkArgument(isValidIdentifier(from), "Invalid identifier:
%s", from);
Review Comment:
Intent here was to have common exception for `Table/View`. But I think with
`View` impl we will have to revisit this.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533712833
##
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##
@@ -309,65 +304,39 @@ protected enum CommitStatus {
* @return Commit Status of Success, Failure or Unknown
*/
protected CommitStatus checkCommitStatus(String newMetadataLocation,
TableMetadata config) {
-int maxAttempts =
-PropertyUtil.propertyAsInt(
-config.properties(), COMMIT_NUM_STATUS_CHECKS,
COMMIT_NUM_STATUS_CHECKS_DEFAULT);
-long minWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS,
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS_DEFAULT);
-long maxWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS,
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS_DEFAULT);
-long totalRetryMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS,
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS_DEFAULT);
-
-AtomicReference status = new
AtomicReference<>(CommitStatus.UNKNOWN);
-
-Tasks.foreach(newMetadataLocation)
-.retry(maxAttempts)
-.suppressFailureWhenFinished()
-.exponentialBackoff(minWaitMs, maxWaitMs, totalRetryMs, 2.0)
-.onFailure(
-(location, checkException) ->
-LOG.error("Cannot check if commit to {} exists.", tableName(),
checkException))
-.run(
-location -> {
- TableMetadata metadata = refresh();
- String currentMetadataFileLocation =
metadata.metadataFileLocation();
- boolean commitSuccess =
- currentMetadataFileLocation.equals(newMetadataLocation)
- || metadata.previousFiles().stream()
- .anyMatch(log ->
log.file().equals(newMetadataLocation));
- if (commitSuccess) {
-LOG.info(
-"Commit status check: Commit to {} of {} succeeded",
-tableName(),
-newMetadataLocation);
-status.set(CommitStatus.SUCCESS);
- } else {
-LOG.warn(
-"Commit status check: Commit to {} of {} unknown, new
metadata location is not current "
-+ "or in history",
-tableName(),
-newMetadataLocation);
- }
-});
-
-if (status.get() == CommitStatus.UNKNOWN) {
- LOG.error(
- "Cannot determine commit state to {}. Failed during checking {}
times. "
- + "Treating commit state as unknown.",
- tableName(),
- maxAttempts);
+return CommitStatus.valueOf(
+checkCommitStatus(
+tableName(),
+newMetadataLocation,
+config.properties(),
+() -> checkCurrentMetadataLocation(newMetadataLocation))
+.name());
+ }
+
+ /**
+ * Checks the new metadata location presents or not after refreshing the
table.
+ *
+ * @param newMetadataLocation newly written metadata location
+ * @return true if the new metadata location presents with current or
previous metadata files.
+ */
+ protected boolean checkCurrentMetadataLocation(String newMetadataLocation) {
Review Comment:
Not `Private` , bcoz It should be available outside of the core-package. Not
`public` as it should be only scoped too child class. Do we need `public`
privilege here ?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533702231
##
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##
@@ -136,6 +138,18 @@ public static void dropTableData(FileIO io, TableMetadata
metadata) {
deleteFile(io, metadata.metadataFileLocation(), "metadata");
}
+ /**
+ * Drops view metadata files referenced by ViewMetadata.
+ *
+ * This should be called by dropView implementations
+ *
+ * @param io a FileIO to use for deletes
+ * @param metadata the last valid ViewMetadata instance for a dropped view.
+ */
+ public static void dropViewMetadata(FileIO io, ViewMetadata metadata) {
Review Comment:
My Bad, I missed to push the corresponding commit.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533432188
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##
@@ -166,168 +159,36 @@ protected void doRefresh() {
refreshFromMetadataLocation(metadataLocation, metadataRefreshMaxRetries);
}
- @SuppressWarnings("checkstyle:CyclomaticComplexity")
@Override
protected void doCommit(TableMetadata base, TableMetadata metadata) {
boolean newTable = base == null;
String newMetadataLocation = writeNewMetadataIfRequired(newTable,
metadata);
-boolean hiveEngineEnabled = hiveEngineEnabled(metadata, conf);
Review Comment:
see my comment in
https://github.com/apache/iceberg/pull/10001/files#r1533430710. Please revert
these changes here. Once view support is in, we can revisit what we'll extract
out
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533430710
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java:
##
@@ -181,4 +272,203 @@ default Table newHmsTable(String hmsTableOwner) {
return newTable;
}
+
+ @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ default void commitWithHiveLock(
Review Comment:
can we revert any changes in this class starting from L276 to L473? I feel
like we're extracting too much functionality into a common place and some
things are slightly different for tables/views later.
That's why I think it would be better to revert L276 to L473 at this point.
In the first iteration of view support it's probably ok if some things are
duplicated and once View support for Hive is in, we can do another round of
refactoring to extract what's really common
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533422080
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java:
##
@@ -139,22 +212,40 @@ static StorageDescriptor storageDescriptor(TableMetadata
metadata, boolean hiveE
storageDescriptor.setInputFormat("org.apache.hadoop.mapred.FileInputFormat");
serDeInfo.setSerializationLib("org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe");
}
+
storageDescriptor.setSerdeInfo(serDeInfo);
return storageDescriptor;
}
- static void cleanupMetadata(FileIO io, String commitStatus, String
metadataLocation) {
+ default void cleanupMetadataAndUnlock(
+ HiveLock lock,
+ FileIO io,
+ BaseMetastoreOperations.CommitStatus commitStatus,
+ String metadataLocation) {
try {
- if (commitStatus.equalsIgnoreCase("FAILURE")) {
+ if (commitStatus.name().equalsIgnoreCase("FAILURE")) {
// If we are sure the commit failed, clean up the uncommitted metadata
file
io.deleteFile(metadataLocation);
}
} catch (RuntimeException e) {
LOG.error("Failed to cleanup metadata file at {}", metadataLocation, e);
+} finally {
+ lock.unlock();
}
}
- default Table newHmsTable(String hmsTableOwner) {
+ default HiveLock lockObject(
+ Map properties, Configuration conf, String catalogName) {
+if (hiveLockEnabled(conf, properties)) {
+ return new MetastoreLock(conf, metaClients(), catalogName, database(),
table());
+} else {
+ return new NoLock();
+}
+ }
+
+ default Table newHmsTable(Map properties) {
Review Comment:
why do we need to change this method's parameter?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533419675
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java:
##
@@ -139,22 +212,40 @@ static StorageDescriptor storageDescriptor(TableMetadata
metadata, boolean hiveE
storageDescriptor.setInputFormat("org.apache.hadoop.mapred.FileInputFormat");
serDeInfo.setSerializationLib("org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe");
}
+
storageDescriptor.setSerdeInfo(serDeInfo);
return storageDescriptor;
}
- static void cleanupMetadata(FileIO io, String commitStatus, String
metadataLocation) {
+ default void cleanupMetadataAndUnlock(
Review Comment:
I would probably just leave `cleanupMetadata` as it is and introduce
`cleanupMetadataAndUnlock()` as a separate method
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533415487
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -250,7 +259,7 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
return null;
});
- LOG.info("Renamed table from {}, to {}", from, to);
+ LOG.info("Renamed {} from {}, to {}", contentType.name(), from, to);
Review Comment:
should this not be using `contentType.value()` instead of `name()`?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533414139
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -222,9 +211,29 @@ public boolean dropTable(TableIdentifier identifier,
boolean purge) {
@Override
public void renameTable(TableIdentifier from, TableIdentifier originalTo) {
-if (!isValidIdentifier(from)) {
- throw new NoSuchTableException("Invalid identifier: %s", from);
-}
+renameContent(from, originalTo, HiveOperationsBase.ContentType.TABLE);
+ }
+
+ private List listIcebergTables(
+ List tableNames, Namespace namespace, String tableTypeProp)
+ throws TException, InterruptedException {
+List tableObjects =
+clients.run(client -> client.getTableObjectsByName(namespace.level(0),
tableNames));
+return tableObjects.stream()
+.filter(
+table ->
+table.getParameters() != null
+&& tableTypeProp.equalsIgnoreCase(
+
table.getParameters().get(BaseMetastoreTableOperations.TABLE_TYPE_PROP)))
+.map(table -> TableIdentifier.of(namespace, table.getTableName()))
+.collect(Collectors.toList());
+ }
+
+ private void renameContent(
+ TableIdentifier from,
+ TableIdentifier originalTo,
+ HiveOperationsBase.ContentType contentType) {
+Preconditions.checkArgument(isValidIdentifier(from), "Invalid identifier:
%s", from);
Review Comment:
this is changing existing behavior. Previously it would throw
`NoSuchTableException`
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533412107
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -222,9 +211,29 @@ public boolean dropTable(TableIdentifier identifier,
boolean purge) {
@Override
public void renameTable(TableIdentifier from, TableIdentifier originalTo) {
-if (!isValidIdentifier(from)) {
- throw new NoSuchTableException("Invalid identifier: %s", from);
-}
+renameContent(from, originalTo, HiveOperationsBase.ContentType.TABLE);
+ }
+
+ private List listIcebergTables(
+ List tableNames, Namespace namespace, String tableTypeProp)
+ throws TException, InterruptedException {
+List tableObjects =
+clients.run(client -> client.getTableObjectsByName(namespace.level(0),
tableNames));
+return tableObjects.stream()
+.filter(
+table ->
+table.getParameters() != null
+&& tableTypeProp.equalsIgnoreCase(
+
table.getParameters().get(BaseMetastoreTableOperations.TABLE_TYPE_PROP)))
+.map(table -> TableIdentifier.of(namespace, table.getTableName()))
+.collect(Collectors.toList());
+ }
+
+ private void renameContent(
Review Comment:
```suggestion
private void renameTableOrView(
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533411887
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -273,6 +282,21 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
}
}
+ private void validateTable(
Review Comment:
this feels like it should be named `validateTableIsIcebergTableOrView`
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533378904
##
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##
@@ -136,6 +138,18 @@ public static void dropTableData(FileIO io, TableMetadata
metadata) {
deleteFile(io, metadata.metadataFileLocation(), "metadata");
}
+ /**
+ * Drops view metadata files referenced by ViewMetadata.
+ *
+ * This should be called by dropView implementations
+ *
+ * @param io a FileIO to use for deletes
+ * @param metadata the last valid ViewMetadata instance for a dropped view.
+ */
+ public static void dropViewMetadata(FileIO io, ViewMetadata metadata) {
Review Comment:
this hasn't been addressed yet
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533378394
##
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##
@@ -309,65 +304,39 @@ protected enum CommitStatus {
* @return Commit Status of Success, Failure or Unknown
*/
protected CommitStatus checkCommitStatus(String newMetadataLocation,
TableMetadata config) {
-int maxAttempts =
-PropertyUtil.propertyAsInt(
-config.properties(), COMMIT_NUM_STATUS_CHECKS,
COMMIT_NUM_STATUS_CHECKS_DEFAULT);
-long minWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS,
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS_DEFAULT);
-long maxWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS,
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS_DEFAULT);
-long totalRetryMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS,
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS_DEFAULT);
-
-AtomicReference status = new
AtomicReference<>(CommitStatus.UNKNOWN);
-
-Tasks.foreach(newMetadataLocation)
-.retry(maxAttempts)
-.suppressFailureWhenFinished()
-.exponentialBackoff(minWaitMs, maxWaitMs, totalRetryMs, 2.0)
-.onFailure(
-(location, checkException) ->
-LOG.error("Cannot check if commit to {} exists.", tableName(),
checkException))
-.run(
-location -> {
- TableMetadata metadata = refresh();
- String currentMetadataFileLocation =
metadata.metadataFileLocation();
- boolean commitSuccess =
- currentMetadataFileLocation.equals(newMetadataLocation)
- || metadata.previousFiles().stream()
- .anyMatch(log ->
log.file().equals(newMetadataLocation));
- if (commitSuccess) {
-LOG.info(
-"Commit status check: Commit to {} of {} succeeded",
-tableName(),
-newMetadataLocation);
-status.set(CommitStatus.SUCCESS);
- } else {
-LOG.warn(
-"Commit status check: Commit to {} of {} unknown, new
metadata location is not current "
-+ "or in history",
-tableName(),
-newMetadataLocation);
- }
-});
-
-if (status.get() == CommitStatus.UNKNOWN) {
- LOG.error(
- "Cannot determine commit state to {}. Failed during checking {}
times. "
- + "Treating commit state as unknown.",
- tableName(),
- maxAttempts);
+return CommitStatus.valueOf(
+checkCommitStatus(
+tableName(),
+newMetadataLocation,
+config.properties(),
+() -> checkCurrentMetadataLocation(newMetadataLocation))
+.name());
+ }
+
+ /**
+ * Checks the new metadata location presents or not after refreshing the
table.
+ *
+ * @param newMetadataLocation newly written metadata location
+ * @return true if the new metadata location presents with current or
previous metadata files.
Review Comment:
```suggestion
* @return true if the new metadata location is the current metadata
location or present within previous metadata files.
```
please also update the javadoc in L317 with a similar sentence as here
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533376457
##
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##
@@ -309,65 +304,39 @@ protected enum CommitStatus {
* @return Commit Status of Success, Failure or Unknown
*/
protected CommitStatus checkCommitStatus(String newMetadataLocation,
TableMetadata config) {
-int maxAttempts =
-PropertyUtil.propertyAsInt(
-config.properties(), COMMIT_NUM_STATUS_CHECKS,
COMMIT_NUM_STATUS_CHECKS_DEFAULT);
-long minWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS,
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS_DEFAULT);
-long maxWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS,
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS_DEFAULT);
-long totalRetryMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS,
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS_DEFAULT);
-
-AtomicReference status = new
AtomicReference<>(CommitStatus.UNKNOWN);
-
-Tasks.foreach(newMetadataLocation)
-.retry(maxAttempts)
-.suppressFailureWhenFinished()
-.exponentialBackoff(minWaitMs, maxWaitMs, totalRetryMs, 2.0)
-.onFailure(
-(location, checkException) ->
-LOG.error("Cannot check if commit to {} exists.", tableName(),
checkException))
-.run(
-location -> {
- TableMetadata metadata = refresh();
- String currentMetadataFileLocation =
metadata.metadataFileLocation();
- boolean commitSuccess =
- currentMetadataFileLocation.equals(newMetadataLocation)
- || metadata.previousFiles().stream()
- .anyMatch(log ->
log.file().equals(newMetadataLocation));
- if (commitSuccess) {
-LOG.info(
-"Commit status check: Commit to {} of {} succeeded",
-tableName(),
-newMetadataLocation);
-status.set(CommitStatus.SUCCESS);
- } else {
-LOG.warn(
-"Commit status check: Commit to {} of {} unknown, new
metadata location is not current "
-+ "or in history",
-tableName(),
-newMetadataLocation);
- }
-});
-
-if (status.get() == CommitStatus.UNKNOWN) {
- LOG.error(
- "Cannot determine commit state to {}. Failed during checking {}
times. "
- + "Treating commit state as unknown.",
- tableName(),
- maxAttempts);
+return CommitStatus.valueOf(
+checkCommitStatus(
+tableName(),
+newMetadataLocation,
+config.properties(),
+() -> checkCurrentMetadataLocation(newMetadataLocation))
+.name());
+ }
+
+ /**
+ * Checks the new metadata location presents or not after refreshing the
table.
+ *
+ * @param newMetadataLocation newly written metadata location
+ * @return true if the new metadata location presents with current or
previous metadata files.
+ */
+ protected boolean checkCurrentMetadataLocation(String newMetadataLocation) {
+TableMetadata metadata = refresh();
+Preconditions.checkNotNull(metadata, "Unexpected null table metadata");
Review Comment:
this hasn't been addressed yet
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533376150
##
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##
@@ -309,65 +304,39 @@ protected enum CommitStatus {
* @return Commit Status of Success, Failure or Unknown
*/
protected CommitStatus checkCommitStatus(String newMetadataLocation,
TableMetadata config) {
-int maxAttempts =
-PropertyUtil.propertyAsInt(
-config.properties(), COMMIT_NUM_STATUS_CHECKS,
COMMIT_NUM_STATUS_CHECKS_DEFAULT);
-long minWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS,
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS_DEFAULT);
-long maxWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS,
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS_DEFAULT);
-long totalRetryMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS,
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS_DEFAULT);
-
-AtomicReference status = new
AtomicReference<>(CommitStatus.UNKNOWN);
-
-Tasks.foreach(newMetadataLocation)
-.retry(maxAttempts)
-.suppressFailureWhenFinished()
-.exponentialBackoff(minWaitMs, maxWaitMs, totalRetryMs, 2.0)
-.onFailure(
-(location, checkException) ->
-LOG.error("Cannot check if commit to {} exists.", tableName(),
checkException))
-.run(
-location -> {
- TableMetadata metadata = refresh();
- String currentMetadataFileLocation =
metadata.metadataFileLocation();
- boolean commitSuccess =
- currentMetadataFileLocation.equals(newMetadataLocation)
- || metadata.previousFiles().stream()
- .anyMatch(log ->
log.file().equals(newMetadataLocation));
- if (commitSuccess) {
-LOG.info(
-"Commit status check: Commit to {} of {} succeeded",
-tableName(),
-newMetadataLocation);
-status.set(CommitStatus.SUCCESS);
- } else {
-LOG.warn(
-"Commit status check: Commit to {} of {} unknown, new
metadata location is not current "
-+ "or in history",
-tableName(),
-newMetadataLocation);
- }
-});
-
-if (status.get() == CommitStatus.UNKNOWN) {
- LOG.error(
- "Cannot determine commit state to {}. Failed during checking {}
times. "
- + "Treating commit state as unknown.",
- tableName(),
- maxAttempts);
+return CommitStatus.valueOf(
+checkCommitStatus(
+tableName(),
+newMetadataLocation,
+config.properties(),
+() -> checkCurrentMetadataLocation(newMetadataLocation))
+.name());
+ }
+
+ /**
+ * Checks the new metadata location presents or not after refreshing the
table.
+ *
+ * @param newMetadataLocation newly written metadata location
+ * @return true if the new metadata location presents with current or
previous metadata files.
+ */
+ protected boolean checkCurrentMetadataLocation(String newMetadataLocation) {
Review Comment:
why does this have to be protected?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533372773
##
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##
@@ -291,6 +284,8 @@ public long newSnapshotId() {
};
}
+ /** @deprecated Use {@link BaseMetastoreOperations.CommitStatus} */
Review Comment:
```suggestion
/** @deprecated since 1.6.0, will be removed in 1.7.0; use {@link
BaseMetastoreOperations.CommitStatus} instead */
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533370990
##
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.apache.iceberg.TableProperties.COMMIT_NUM_STATUS_CHECKS;
+import static
org.apache.iceberg.TableProperties.COMMIT_NUM_STATUS_CHECKS_DEFAULT;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS_DEFAULT;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS_DEFAULT;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS_DEFAULT;
+
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+import org.apache.iceberg.util.PropertyUtil;
+import org.apache.iceberg.util.Tasks;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class BaseMetastoreOperations {
+ private static final Logger LOG =
LoggerFactory.getLogger(BaseMetastoreOperations.class);
+
+ public enum CommitStatus {
+FAILURE,
+SUCCESS,
+UNKNOWN
+ }
+
+ /**
+ * Attempt to load the content and see if any current or past metadata
location matches the one we
+ * were attempting to set. This is used as a last resort when we are dealing
with exceptions that
+ * may indicate the commit has failed but don't have proof that this is the
case. Note that all
+ * the previous locations must also be searched on the chance that a second
committer was able to
+ * successfully commit on top of our commit.
+ *
+ * @param contentName full name of the content
+ * @param newMetadataLocation the path of the new commit file
+ * @param properties properties for retry
+ * @param newMetadataCheckSupplier check if the latest metadata presents or
not using metadata
+ * location for table, version id for view.
+ * @return Commit Status of Success, Failure or Unknown
+ */
+ protected CommitStatus checkCommitStatus(
+ String contentName,
+ String newMetadataLocation,
+ Map properties,
+ Supplier newMetadataCheckSupplier) {
Review Comment:
```suggestion
Supplier commitStatusSupplier) {
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1533369953
##
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.apache.iceberg.TableProperties.COMMIT_NUM_STATUS_CHECKS;
+import static
org.apache.iceberg.TableProperties.COMMIT_NUM_STATUS_CHECKS_DEFAULT;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_MAX_WAIT_MS_DEFAULT;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_MIN_WAIT_MS_DEFAULT;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS;
+import static
org.apache.iceberg.TableProperties.COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS_DEFAULT;
+
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+import org.apache.iceberg.util.PropertyUtil;
+import org.apache.iceberg.util.Tasks;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class BaseMetastoreOperations {
+ private static final Logger LOG =
LoggerFactory.getLogger(BaseMetastoreOperations.class);
+
+ public enum CommitStatus {
+FAILURE,
+SUCCESS,
+UNKNOWN
+ }
+
+ /**
+ * Attempt to load the content and see if any current or past metadata
location matches the one we
+ * were attempting to set. This is used as a last resort when we are dealing
with exceptions that
+ * may indicate the commit has failed but don't have proof that this is the
case. Note that all
+ * the previous locations must also be searched on the chance that a second
committer was able to
+ * successfully commit on top of our commit.
+ *
+ * @param contentName full name of the content
+ * @param newMetadataLocation the path of the new commit file
+ * @param properties properties for retry
+ * @param newMetadataCheckSupplier check if the latest metadata presents or
not using metadata
+ * location for table, version id for view.
+ * @return Commit Status of Success, Failure or Unknown
+ */
+ protected CommitStatus checkCommitStatus(
+ String contentName,
Review Comment:
it's not clear what `contentName` refers to. So I would rename this to
`tableOrViewName` (and also reflect this in the javadoc in L54
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on PR #10001: URL: https://github.com/apache/iceberg/pull/10001#issuecomment-2011254887 @nastra , Please let me know if there are more comments ? -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1530379477
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -250,29 +264,66 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
return null;
});
- LOG.info("Renamed table from {}, to {}", from, to);
+ LOG.info("Renamed {} from {}, to {}", contentType.name(),
fromIdentifier, to);
} catch (NoSuchObjectException e) {
- throw new NoSuchTableException("Table does not exist: %s", from);
+ switch (contentType) {
+case TABLE:
+ throw new NoSuchTableException(
+ "Cannot rename %s to %s. Table does not exist", fromIdentifier,
to);
+case VIEW:
+ throw new NoSuchViewException(
+ "Cannot rename %s to %s. View does not exist", fromIdentifier,
to);
+ }
} catch (InvalidOperationException e) {
if (e.getMessage() != null
&& e.getMessage().contains(String.format("new table %s already
exists", to))) {
-throw new org.apache.iceberg.exceptions.AlreadyExistsException(
-"Table already exists: %s", to);
+throwErrorForExistedToContent(fromIdentifier,
removeCatalogName(toIdentifier));
Review Comment:
what I mean is that we shouldn't be doing whather it's doing in
`throwErrorForExistedToContent()` at this point and just throw the
`AlreadyExistsException` as it was done previously. We don't want do add new
functionality in this PR IMO
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1530375282
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java:
##
@@ -169,8 +260,8 @@ default Table newHmsTable(String hmsTableOwner) {
null,
Collections.emptyList(),
Maps.newHashMap(),
-null,
-null,
+properties.getOrDefault(HiveCatalog.VIEW_ORIGINAL_TEXT, null),
Review Comment:
I think this PR should be a pure refactoring and not add new
things/functionality. The current code on `main` doesn't look at this property,
so why do we need to check this property as part of this PR?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1530321209
##
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##
@@ -309,65 +304,39 @@ protected enum CommitStatus {
* @return Commit Status of Success, Failure or Unknown
*/
protected CommitStatus checkCommitStatus(String newMetadataLocation,
TableMetadata config) {
-int maxAttempts =
-PropertyUtil.propertyAsInt(
-config.properties(), COMMIT_NUM_STATUS_CHECKS,
COMMIT_NUM_STATUS_CHECKS_DEFAULT);
-long minWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS,
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS_DEFAULT);
-long maxWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS,
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS_DEFAULT);
-long totalRetryMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS,
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS_DEFAULT);
-
-AtomicReference status = new
AtomicReference<>(CommitStatus.UNKNOWN);
-
-Tasks.foreach(newMetadataLocation)
-.retry(maxAttempts)
-.suppressFailureWhenFinished()
-.exponentialBackoff(minWaitMs, maxWaitMs, totalRetryMs, 2.0)
-.onFailure(
-(location, checkException) ->
-LOG.error("Cannot check if commit to {} exists.", tableName(),
checkException))
-.run(
-location -> {
- TableMetadata metadata = refresh();
- String currentMetadataFileLocation =
metadata.metadataFileLocation();
- boolean commitSuccess =
- currentMetadataFileLocation.equals(newMetadataLocation)
- || metadata.previousFiles().stream()
- .anyMatch(log ->
log.file().equals(newMetadataLocation));
- if (commitSuccess) {
-LOG.info(
-"Commit status check: Commit to {} of {} succeeded",
-tableName(),
-newMetadataLocation);
-status.set(CommitStatus.SUCCESS);
- } else {
-LOG.warn(
-"Commit status check: Commit to {} of {} unknown, new
metadata location is not current "
-+ "or in history",
-tableName(),
-newMetadataLocation);
- }
-});
-
-if (status.get() == CommitStatus.UNKNOWN) {
- LOG.error(
- "Cannot determine commit state to {}. Failed during checking {}
times. "
- + "Treating commit state as unknown.",
- tableName(),
- maxAttempts);
+return CommitStatus.valueOf(
+checkCommitStatus(
+tableName(),
+newMetadataLocation,
+config.properties(),
+() -> checkCurrentMetadataLocation(newMetadataLocation))
+.name());
+ }
+
+ /**
+ * Checks the new metadata location presents or not after refreshing the
table.
+ *
+ * @param newMetadataLocation newly written metadata location
+ * @return true if the new metadata location presents with current or
previous metadata files.
+ */
+ protected boolean checkCurrentMetadataLocation(String newMetadataLocation) {
+TableMetadata metadata = refresh();
+Preconditions.checkNotNull(metadata, "Unexpected null table metadata");
Review Comment:
Earlier reason was to return all the metadata files and validate during
commit. But I moved entire validation with supplier. I will revert to the
original one.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1530317976
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java:
##
@@ -169,8 +260,8 @@ default Table newHmsTable(String hmsTableOwner) {
null,
Collections.emptyList(),
Maps.newHashMap(),
-null,
-null,
+properties.getOrDefault(HiveCatalog.VIEW_ORIGINAL_TEXT, null),
Review Comment:
I think this might not be specific to Hive-View. This is another field in
Hive-Table . IMO, This field either we can ignore for both iceberg-table and
view or make it optional with properties.
Ref:
https://github.com/apache/hive/blame/df45194268ffb10f9f7aae0ab4e3aec35a7b31d8/standalone-metastore/metastore-common/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/Table.java#L23
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -250,29 +264,66 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
return null;
});
- LOG.info("Renamed table from {}, to {}", from, to);
+ LOG.info("Renamed {} from {}, to {}", contentType.name(),
fromIdentifier, to);
} catch (NoSuchObjectException e) {
- throw new NoSuchTableException("Table does not exist: %s", from);
+ switch (contentType) {
+case TABLE:
+ throw new NoSuchTableException(
+ "Cannot rename %s to %s. Table does not exist", fromIdentifier,
to);
+case VIEW:
+ throw new NoSuchViewException(
+ "Cannot rename %s to %s. View does not exist", fromIdentifier,
to);
+ }
} catch (InvalidOperationException e) {
if (e.getMessage() != null
&& e.getMessage().contains(String.format("new table %s already
exists", to))) {
-throw new org.apache.iceberg.exceptions.AlreadyExistsException(
-"Table already exists: %s", to);
+throwErrorForExistedToContent(fromIdentifier,
removeCatalogName(toIdentifier));
Review Comment:
Motivation behind this changes is
`ViewCatalogTests` expects error message like `Cannot rename %s to %s. %s
already exists`. It should explicitly tell the upstream **TO** tableType.
Also we are making only one call to retrieve TO content.
Nevertheless, What do you propose to match the error messages for
`ViewCatalogTests` ?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1530154379
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java:
##
@@ -169,8 +260,8 @@ default Table newHmsTable(String hmsTableOwner) {
null,
Collections.emptyList(),
Maps.newHashMap(),
-null,
-null,
+properties.getOrDefault(HiveCatalog.VIEW_ORIGINAL_TEXT, null),
Review Comment:
why is this adding view-specific things?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1530152090
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -250,29 +264,66 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
return null;
});
- LOG.info("Renamed table from {}, to {}", from, to);
+ LOG.info("Renamed {} from {}, to {}", contentType.name(),
fromIdentifier, to);
} catch (NoSuchObjectException e) {
- throw new NoSuchTableException("Table does not exist: %s", from);
+ switch (contentType) {
+case TABLE:
+ throw new NoSuchTableException(
+ "Cannot rename %s to %s. Table does not exist", fromIdentifier,
to);
+case VIEW:
+ throw new NoSuchViewException(
+ "Cannot rename %s to %s. View does not exist", fromIdentifier,
to);
+ }
} catch (InvalidOperationException e) {
if (e.getMessage() != null
&& e.getMessage().contains(String.format("new table %s already
exists", to))) {
-throw new org.apache.iceberg.exceptions.AlreadyExistsException(
-"Table already exists: %s", to);
+throwErrorForExistedToContent(fromIdentifier,
removeCatalogName(toIdentifier));
Review Comment:
I don't think we want to do this here, as this would do another call to
retrieve the table. I would leave the code here as it was before
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1530149331
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -222,24 +214,46 @@ public boolean dropTable(TableIdentifier identifier,
boolean purge) {
@Override
public void renameTable(TableIdentifier from, TableIdentifier originalTo) {
-if (!isValidIdentifier(from)) {
- throw new NoSuchTableException("Invalid identifier: %s", from);
-}
+renameContent(from, originalTo, HiveOperationsBase.ContentType.TABLE);
+ }
-TableIdentifier to = removeCatalogName(originalTo);
+ private List filterIcebergEntities(
+ List tableNames, Namespace namespace, String tableTypeProp)
+ throws TException, InterruptedException {
+List tableObjects =
+clients.run(client -> client.getTableObjectsByName(namespace.level(0),
tableNames));
+return tableObjects.stream()
+.filter(
+table ->
+table.getParameters() != null
+&& tableTypeProp.equalsIgnoreCase(
+
table.getParameters().get(BaseMetastoreTableOperations.TABLE_TYPE_PROP)))
+.map(table -> TableIdentifier.of(namespace, table.getTableName()))
+.collect(Collectors.toList());
+ }
+
+ private void renameContent(
+ TableIdentifier fromIdentifier,
+ TableIdentifier toIdentifier,
+ HiveOperationsBase.ContentType contentType) {
+Preconditions.checkArgument(
+isValidIdentifier(fromIdentifier), "Invalid identifier: %s",
fromIdentifier);
+
+TableIdentifier to = removeCatalogName(toIdentifier);
Preconditions.checkArgument(isValidIdentifier(to), "Invalid identifier:
%s", to);
if (!namespaceExists(to.namespace())) {
throw new NoSuchNamespaceException(
- "Cannot rename %s to %s. Namespace does not exist: %s", from, to,
to.namespace());
+ "Cannot rename %s to %s. Namespace does not exist: %s",
+ fromIdentifier, to, to.namespace());
}
String toDatabase = to.namespace().level(0);
-String fromDatabase = from.namespace().level(0);
-String fromName = from.name();
+String fromDatabase = fromIdentifier.namespace().level(0);
Review Comment:
```suggestion
String fromDatabase = from.namespace().level(0);
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1530146675
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -222,24 +214,46 @@ public boolean dropTable(TableIdentifier identifier,
boolean purge) {
@Override
public void renameTable(TableIdentifier from, TableIdentifier originalTo) {
-if (!isValidIdentifier(from)) {
- throw new NoSuchTableException("Invalid identifier: %s", from);
-}
+renameContent(from, originalTo, HiveOperationsBase.ContentType.TABLE);
+ }
-TableIdentifier to = removeCatalogName(originalTo);
+ private List filterIcebergEntities(
+ List tableNames, Namespace namespace, String tableTypeProp)
+ throws TException, InterruptedException {
+List tableObjects =
+clients.run(client -> client.getTableObjectsByName(namespace.level(0),
tableNames));
+return tableObjects.stream()
+.filter(
+table ->
+table.getParameters() != null
+&& tableTypeProp.equalsIgnoreCase(
+
table.getParameters().get(BaseMetastoreTableOperations.TABLE_TYPE_PROP)))
+.map(table -> TableIdentifier.of(namespace, table.getTableName()))
+.collect(Collectors.toList());
+ }
+
+ private void renameContent(
+ TableIdentifier fromIdentifier,
+ TableIdentifier toIdentifier,
+ HiveOperationsBase.ContentType contentType) {
+Preconditions.checkArgument(
+isValidIdentifier(fromIdentifier), "Invalid identifier: %s",
fromIdentifier);
+
+TableIdentifier to = removeCatalogName(toIdentifier);
Preconditions.checkArgument(isValidIdentifier(to), "Invalid identifier:
%s", to);
if (!namespaceExists(to.namespace())) {
throw new NoSuchNamespaceException(
- "Cannot rename %s to %s. Namespace does not exist: %s", from, to,
to.namespace());
+ "Cannot rename %s to %s. Namespace does not exist: %s",
+ fromIdentifier, to, to.namespace());
}
String toDatabase = to.namespace().level(0);
-String fromDatabase = from.namespace().level(0);
-String fromName = from.name();
+String fromDatabase = fromIdentifier.namespace().level(0);
Review Comment:
why is the rename required here?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1530145798
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -222,24 +214,46 @@ public boolean dropTable(TableIdentifier identifier,
boolean purge) {
@Override
public void renameTable(TableIdentifier from, TableIdentifier originalTo) {
-if (!isValidIdentifier(from)) {
- throw new NoSuchTableException("Invalid identifier: %s", from);
-}
+renameContent(from, originalTo, HiveOperationsBase.ContentType.TABLE);
+ }
-TableIdentifier to = removeCatalogName(originalTo);
+ private List filterIcebergEntities(
Review Comment:
```suggestion
private List listIcebergTables(
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1530141957
##
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##
@@ -136,6 +138,18 @@ public static void dropTableData(FileIO io, TableMetadata
metadata) {
deleteFile(io, metadata.metadataFileLocation(), "metadata");
}
+ /**
+ * Drops view metadata files referenced by ViewMetadata.
+ *
+ * This should be called by dropView implementations
+ *
+ * @param io a FileIO to use for deletes
+ * @param metadata the last valid ViewMetadata instance for a dropped view.
+ */
+ public static void dropViewMetadata(FileIO io, ViewMetadata metadata) {
Review Comment:
from what I understand, this PR only refactors a bunch of things to make it
easier to add view support. That means we probably shouldn't be adding
view-specific things in this PR as we just want to move stuff around and
provide the exact same code behavior that previously existed, right?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1530138582
##
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##
@@ -309,65 +304,39 @@ protected enum CommitStatus {
* @return Commit Status of Success, Failure or Unknown
*/
protected CommitStatus checkCommitStatus(String newMetadataLocation,
TableMetadata config) {
-int maxAttempts =
-PropertyUtil.propertyAsInt(
-config.properties(), COMMIT_NUM_STATUS_CHECKS,
COMMIT_NUM_STATUS_CHECKS_DEFAULT);
-long minWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS,
-COMMIT_STATUS_CHECKS_MIN_WAIT_MS_DEFAULT);
-long maxWaitMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS,
-COMMIT_STATUS_CHECKS_MAX_WAIT_MS_DEFAULT);
-long totalRetryMs =
-PropertyUtil.propertyAsLong(
-config.properties(),
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS,
-COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS_DEFAULT);
-
-AtomicReference status = new
AtomicReference<>(CommitStatus.UNKNOWN);
-
-Tasks.foreach(newMetadataLocation)
-.retry(maxAttempts)
-.suppressFailureWhenFinished()
-.exponentialBackoff(minWaitMs, maxWaitMs, totalRetryMs, 2.0)
-.onFailure(
-(location, checkException) ->
-LOG.error("Cannot check if commit to {} exists.", tableName(),
checkException))
-.run(
-location -> {
- TableMetadata metadata = refresh();
- String currentMetadataFileLocation =
metadata.metadataFileLocation();
- boolean commitSuccess =
- currentMetadataFileLocation.equals(newMetadataLocation)
- || metadata.previousFiles().stream()
- .anyMatch(log ->
log.file().equals(newMetadataLocation));
- if (commitSuccess) {
-LOG.info(
-"Commit status check: Commit to {} of {} succeeded",
-tableName(),
-newMetadataLocation);
-status.set(CommitStatus.SUCCESS);
- } else {
-LOG.warn(
-"Commit status check: Commit to {} of {} unknown, new
metadata location is not current "
-+ "or in history",
-tableName(),
-newMetadataLocation);
- }
-});
-
-if (status.get() == CommitStatus.UNKNOWN) {
- LOG.error(
- "Cannot determine commit state to {}. Failed during checking {}
times. "
- + "Treating commit state as unknown.",
- tableName(),
- maxAttempts);
+return CommitStatus.valueOf(
+checkCommitStatus(
+tableName(),
+newMetadataLocation,
+config.properties(),
+() -> checkCurrentMetadataLocation(newMetadataLocation))
+.name());
+ }
+
+ /**
+ * Checks the new metadata location presents or not after refreshing the
table.
+ *
+ * @param newMetadataLocation newly written metadata location
+ * @return true if the new metadata location presents with current or
previous metadata files.
+ */
+ protected boolean checkCurrentMetadataLocation(String newMetadataLocation) {
+TableMetadata metadata = refresh();
+Preconditions.checkNotNull(metadata, "Unexpected null table metadata");
Review Comment:
I would have expected this code to be what the previous code was:
```
TableMetadata metadata = refresh();
String currentMetadataFileLocation = metadata.metadataFileLocation();
boolean commitSuccess =
currentMetadataFileLocation.equals(newMetadataLocation)
|| metadata.previousFiles().stream().anyMatch(log ->
log.file().equals(newMetadataLocation));
```
can you elaborate why the code here now ends up being different?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1529998283
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##
@@ -166,168 +159,36 @@ protected void doRefresh() {
refreshFromMetadataLocation(metadataLocation, metadataRefreshMaxRetries);
}
- @SuppressWarnings("checkstyle:CyclomaticComplexity")
@Override
protected void doCommit(TableMetadata base, TableMetadata metadata) {
boolean newTable = base == null;
String newMetadataLocation = writeNewMetadataIfRequired(newTable,
metadata);
-boolean hiveEngineEnabled = hiveEngineEnabled(metadata, conf);
Review Comment:
All this common code has been moved to `HiveOperationsBase` .
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1529997210
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java:
##
@@ -181,4 +272,203 @@ default Table newHmsTable(String hmsTableOwner) {
return newTable;
}
+
+ @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ default void commitWithHiveLock(
Review Comment:
This is a common code for committing `Iceberg-Table` and `Iceberg-View `on
`Hive`.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1529993318
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -250,29 +264,66 @@ public void renameTable(TableIdentifier from,
TableIdentifier originalTo) {
return null;
});
- LOG.info("Renamed table from {}, to {}", from, to);
+ LOG.info("Renamed {} from {}, to {}", contentType.name(),
fromIdentifier, to);
} catch (NoSuchObjectException e) {
- throw new NoSuchTableException("Table does not exist: %s", from);
+ switch (contentType) {
+case TABLE:
+ throw new NoSuchTableException(
+ "Cannot rename %s to %s. Table does not exist", fromIdentifier,
to);
+case VIEW:
+ throw new NoSuchViewException(
+ "Cannot rename %s to %s. View does not exist", fromIdentifier,
to);
+ }
} catch (InvalidOperationException e) {
if (e.getMessage() != null
&& e.getMessage().contains(String.format("new table %s already
exists", to))) {
-throw new org.apache.iceberg.exceptions.AlreadyExistsException(
-"Table already exists: %s", to);
+throwErrorForExistedToContent(fromIdentifier,
removeCatalogName(toIdentifier));
} else {
-throw new RuntimeException("Failed to rename " + from + " to " + to,
e);
+throw new RuntimeException("Failed to rename " + fromIdentifier + " to
" + to, e);
}
} catch (TException e) {
- throw new RuntimeException("Failed to rename " + from + " to " + to, e);
+ throw new RuntimeException("Failed to rename " + fromIdentifier + " to "
+ to, e);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException("Interrupted in call to rename", e);
}
}
+ private void validateTable(
+ HiveOperationsBase.ContentType contentType,
+ String catalogName,
+ Table table,
+ TableIdentifier identifier) {
+switch (contentType) {
+ case TABLE:
+HiveOperationsBase.validateTableIsIceberg(
+table, CatalogUtil.fullTableName(catalogName, identifier));
+break;
+ case VIEW:
+throw new UnsupportedOperationException("View is not supported.");
Review Comment:
This can be updated with Hive-View .
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1529992108
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -222,24 +214,46 @@ public boolean dropTable(TableIdentifier identifier,
boolean purge) {
@Override
public void renameTable(TableIdentifier from, TableIdentifier originalTo) {
-if (!isValidIdentifier(from)) {
- throw new NoSuchTableException("Invalid identifier: %s", from);
-}
+renameContent(from, originalTo, HiveOperationsBase.ContentType.TABLE);
+ }
-TableIdentifier to = removeCatalogName(originalTo);
+ private List filterIcebergEntities(
+ List tableNames, Namespace namespace, String tableTypeProp)
+ throws TException, InterruptedException {
+List tableObjects =
+clients.run(client -> client.getTableObjectsByName(namespace.level(0),
tableNames));
+return tableObjects.stream()
+.filter(
+table ->
+table.getParameters() != null
+&& tableTypeProp.equalsIgnoreCase(
+
table.getParameters().get(BaseMetastoreTableOperations.TABLE_TYPE_PROP)))
+.map(table -> TableIdentifier.of(namespace, table.getTableName()))
+.collect(Collectors.toList());
+ }
+
+ private void renameContent(
Review Comment:
This can be used by view for `renameView` .
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1529991722
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##
@@ -222,24 +214,46 @@ public boolean dropTable(TableIdentifier identifier,
boolean purge) {
@Override
public void renameTable(TableIdentifier from, TableIdentifier originalTo) {
-if (!isValidIdentifier(from)) {
- throw new NoSuchTableException("Invalid identifier: %s", from);
-}
+renameContent(from, originalTo, HiveOperationsBase.ContentType.TABLE);
+ }
-TableIdentifier to = removeCatalogName(originalTo);
+ private List filterIcebergEntities(
Review Comment:
This can be used by view for `listViews` .
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1529989675
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HMSParametersSetter.java:
##
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.hive;
+
+import java.util.Set;
+import org.apache.hadoop.hive.metastore.api.Table;
+
+/** An interface to set the required HMS parameters to the table. */
+interface HMSParametersSetter {
Review Comment:
`Iceberg-table` on Hive and `Iceberg-view `on Hive needs to set few
parameters like `metadatalocation`, `schema`, etc. Few of the metrics needs to
be fetched from `TableMetadata/ViewMetadata.` Since we don't have a common
abstract class for both. This consumer can help to set the respective
parameters with their own implementation .
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1529989675
##
hive-metastore/src/main/java/org/apache/iceberg/hive/HMSParametersSetter.java:
##
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.hive;
+
+import java.util.Set;
+import org.apache.hadoop.hive.metastore.api.Table;
+
+/** An interface to set the required HMS parameters to the table. */
+interface HMSParametersSetter {
Review Comment:
Iceberg table on Hive and Iceberg-view on Hive needs to set few parameters
like metadatalocation, schema, etc. Few of the metrics needs to be fetched from
TableMetadata/ViewMetadata. Since we don't have a common abstract class for
both. This consumer can help to set the respective parameters with their own
implementation .
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1529985583
##
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##
@@ -136,6 +138,18 @@ public static void dropTableData(FileIO io, TableMetadata
metadata) {
deleteFile(io, metadata.metadataFileLocation(), "metadata");
}
+ /**
+ * Drops view metadata files referenced by ViewMetadata.
+ *
+ * This should be called by dropView implementations
+ *
+ * @param io a FileIO to use for deletes
+ * @param metadata the last valid ViewMetadata instance for a dropped view.
+ */
+ public static void dropViewMetadata(FileIO io, ViewMetadata metadata) {
Review Comment:
This change can go as part of Hive-View implementation.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Hive: Arrange common part of the code for Iceberg View. [iceberg]
nk1506 commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1529983789
##
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##
@@ -309,65 +304,39 @@ protected enum CommitStatus {
* @return Commit Status of Success, Failure or Unknown
*/
protected CommitStatus checkCommitStatus(String newMetadataLocation,
TableMetadata config) {
-int maxAttempts =
Review Comment:
Since Hive-View also needs to check the commit status, so it has been moved
to an abstract class `BaseMetastoreOperations`.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
