You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "lirui-apache (via GitHub)" <gi...@apache.org> on 2025/03/25 06:46:33 UTC
[PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
lirui-apache opened a new pull request, #12637:
URL: https://github.com/apache/iceberg/pull/12637
Fixes #11814
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2012247911
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -324,6 +331,24 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
"Committed to table {} with the new metadata location {}", fullName, newMetadataLocation);
}
+ private BaseMetastoreOperations.CommitStatus handlePossibleConcurrentModification(
+ String newMetadataLocation, TableMetadata metadata) {
+ Optional<Boolean> locationCommitted =
+ metadataLocationCommitted(
+ tableName(),
+ newMetadataLocation,
+ metadata.properties(),
+ () -> checkCurrentMetadataLocation(newMetadataLocation));
+ if (locationCommitted.isPresent()) {
+ if (locationCommitted.get()) {
+ return BaseMetastoreOperations.CommitStatus.SUCCESS;
+ } else {
+ return BaseMetastoreOperations.CommitStatus.FAILURE;
+ }
+ }
+ return BaseMetastoreOperations.CommitStatus.UNKNOWN;
+ }
+
Review Comment:
We can get rid of this method, if we have something like `checkCommitStatusStrict` 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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2011775085
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -287,15 +277,25 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
e);
}
- LOG.error(
- "Cannot tell if commit to {}.{} succeeded, attempting to reconnect and check.",
- database,
- tableName,
- e);
- commitStatus = BaseMetastoreOperations.CommitStatus.UNKNOWN;
- commitStatus =
- BaseMetastoreOperations.CommitStatus.valueOf(
- checkCommitStatus(newMetadataLocation, metadata).name());
+ if (e.getMessage() != null
+ && e.getMessage()
Review Comment:
Good catch, will do
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties, commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
Review Comment:
Because `checkCommitStatus` never returns `CommitStatus.FAILURE`, which is required so that we know when to throw a CommitFailedException. And since it's kind of a public API, so I added metadataLocationCommitted which returns false if the new metadata location cannot be found.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -324,6 +324,25 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
"Committed to table {} with the new metadata location {}", fullName, newMetadataLocation);
}
+ private BaseMetastoreOperations.CommitStatus handleConcurrentModification(
+ Throwable throwable, String newMetadataLocation, TableMetadata metadata) {
Review Comment:
Same here as in the previous comment: `checkCommitStatus` doesn't return FAILURE which is required by `handleConcurrentModification `
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2012235572
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##########
@@ -310,7 +310,7 @@ protected CommitStatus checkCommitStatus(String newMetadataLocation, TableMetada
* @return true if the new metadata location is the current metadata location or present within
* previous metadata files.
*/
- private boolean checkCurrentMetadataLocation(String newMetadataLocation) {
+ protected boolean checkCurrentMetadataLocation(String newMetadataLocation) {
Review Comment:
I'm not sure which is better:
1. Making this method `protected`, or
2. Creating a new `checkCommitStatusStrict(String newMetadataLocation, TableMetadata config)` protected 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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2012230419
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties, commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
+ }
+
+ /**
+ * Attempt to load the content and see if any current or past metadata location matches the one we
+ * were attempting to set.
+ *
+ * @param tableOrViewName full name of the Table/View
+ * @param newMetadataLocation the path of the new commit file
+ * @param properties properties for retry
+ * @param commitStatusSupplier check if the latest metadata presents or not using metadata
+ * location for table.
+ * @return Empty if locations cannot be checked, e.g. unable to refresh. True if the new location
+ * is committed, false otherwise.
+ */
+ protected Optional<Boolean> metadataLocationCommitted(
Review Comment:
Instead of returning an `Optional<Boolean>`, would it be better to return the current commit status?
Like:
```
/**
* 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 and don't have proof that this is the case, but we can be sure that
* no retry attempts for the commit will be successful later. 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.
* When the {@code newMetadataLocation} is not in the history the method returns
* {@link CommitStatus.FAILURE}, when the {@link commitStatusSupplier} fails repeatedly the method returns
* {@link CommitStatus.UNKNOWN}.
*
* @param tableOrViewName full name of the Table/View
* @param newMetadataLocation the path of the new commit file
* @param properties properties for retry
* @param commitStatusSupplier check if the latest metadata presents or not using metadata
* location for table.
* @return Commit Status of Success, Failure or Unknown
*/
protected CommitStatus checkCommitStatusStrict(
String tableOrViewName,
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
```
And we need to update the javadoc for the `checkCommitStatus` too, to describe that it will never return `CommitStatus.FAILURE`, like:
```
/**
* 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. When the {@code newMetadataLocation} is not in the
* history or the {@link commitStatusSupplier} fails repeatedly the method returns
* {@link CommitStatus.UNKNOWN}, because possible pending retires might still commit the change.
*
* @param tableOrViewName full name of the Table/View
* @param newMetadataLocation the path of the new commit file
* @param properties properties for retry
* @param commitStatusSupplier check if the latest metadata presents or not using metadata
* location for table.
* @return Commit Status of Success or Unknown
*/
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2020560062
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -287,15 +277,33 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
e);
}
- LOG.error(
- "Cannot tell if commit to {}.{} succeeded, attempting to reconnect and check.",
- database,
- tableName,
- e);
commitStatus = BaseMetastoreOperations.CommitStatus.UNKNOWN;
- commitStatus =
- BaseMetastoreOperations.CommitStatus.valueOf(
- checkCommitStatus(newMetadataLocation, metadata).name());
+ if (e.getMessage() != null
+ && e.getMessage()
+ .contains(
+ "The table has been modified. The parameter value for key '"
+ + HiveTableOperations.METADATA_LOCATION_PROP
+ + "' is")) {
+ // It's possible the HMS client incorrectly retries a successful operation, due to network
+ // issue for example, and triggers this exception. So we need double-check to make sure
+ // this is really a concurrent modification. Hitting this exception means no pending
+ // requests, if any, can succeed later, so it's safe to check status in strict mode
+ commitStatus = checkCommitStatusStrict(newMetadataLocation, metadata);
+ if (commitStatus == BaseMetastoreOperations.CommitStatus.FAILURE) {
+ throw new CommitFailedException(
+ e, "The table %s.%s has been modified concurrently", database, tableName);
+ }
+ } else {
+ LOG.error(
+ "Cannot tell if commit to {}.{} succeeded, attempting to reconnect and check.",
+ database,
+ tableName,
+ e);
+ commitStatus =
Review Comment:
why do we need this weird casting?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2020815560
##########
hive-metastore/src/test/resources/hive-schema-3.1.0.derby.sql:
##########
@@ -130,7 +130,7 @@ CREATE TABLE "APP"."TAB_COL_STATS"(
"BIT_VECTOR" BLOB
);
-CREATE TABLE "APP"."TABLE_PARAMS" ("TBL_ID" BIGINT NOT NULL, "PARAM_KEY" VARCHAR(256) NOT NULL, "PARAM_VALUE" CLOB);
Review Comment:
Good to know the direct SQL is made more robust. Maybe we can revisit this change when we upgrade our hive dependency.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -287,15 +277,33 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
e);
}
- LOG.error(
- "Cannot tell if commit to {}.{} succeeded, attempting to reconnect and check.",
- database,
- tableName,
- e);
commitStatus = BaseMetastoreOperations.CommitStatus.UNKNOWN;
- commitStatus =
- BaseMetastoreOperations.CommitStatus.valueOf(
- checkCommitStatus(newMetadataLocation, metadata).name());
+ if (e.getMessage() != null
+ && e.getMessage()
+ .contains(
+ "The table has been modified. The parameter value for key '"
+ + HiveTableOperations.METADATA_LOCATION_PROP
+ + "' is")) {
+ // It's possible the HMS client incorrectly retries a successful operation, due to network
+ // issue for example, and triggers this exception. So we need double-check to make sure
+ // this is really a concurrent modification. Hitting this exception means no pending
+ // requests, if any, can succeed later, so it's safe to check status in strict mode
+ commitStatus = checkCommitStatusStrict(newMetadataLocation, metadata);
+ if (commitStatus == BaseMetastoreOperations.CommitStatus.FAILURE) {
+ throw new CommitFailedException(
+ e, "The table %s.%s has been modified concurrently", database, tableName);
+ }
+ } else {
+ LOG.error(
+ "Cannot tell if commit to {}.{} succeeded, attempting to reconnect and check.",
+ database,
+ tableName,
+ e);
+ commitStatus =
Review Comment:
I believe it was introduced in #10001 when we had two enums: `BaseMetastoreTableOperations::CommitStatus` and `BaseMetastoreOperations::CommitStatus`, and is not needed anymore. I'd be happy to remove it in this PR if you think that's better.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2013486765
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties, commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
+ }
+
+ /**
+ * Attempt to load the content and see if any current or past metadata location matches the one we
+ * were attempting to set.
+ *
+ * @param tableOrViewName full name of the Table/View
+ * @param newMetadataLocation the path of the new commit file
+ * @param properties properties for retry
+ * @param commitStatusSupplier check if the latest metadata presents or not using metadata
+ * location for table.
+ * @return Empty if locations cannot be checked, e.g. unable to refresh. True if the new location
+ * is committed, false otherwise.
+ */
+ protected Optional<Boolean> metadataLocationCommitted(
Review Comment:
Oh yes, good idea!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2024510734
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -287,15 +277,33 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
e);
}
- LOG.error(
- "Cannot tell if commit to {}.{} succeeded, attempting to reconnect and check.",
- database,
- tableName,
- e);
commitStatus = BaseMetastoreOperations.CommitStatus.UNKNOWN;
- commitStatus =
- BaseMetastoreOperations.CommitStatus.valueOf(
- checkCommitStatus(newMetadataLocation, metadata).name());
+ if (e.getMessage() != null
+ && e.getMessage()
+ .contains(
+ "The table has been modified. The parameter value for key '"
+ + HiveTableOperations.METADATA_LOCATION_PROP
+ + "' is")) {
+ // It's possible the HMS client incorrectly retries a successful operation, due to network
+ // issue for example, and triggers this exception. So we need double-check to make sure
+ // this is really a concurrent modification. Hitting this exception means no pending
+ // requests, if any, can succeed later, so it's safe to check status in strict mode
+ commitStatus = checkCommitStatusStrict(newMetadataLocation, metadata);
+ if (commitStatus == BaseMetastoreOperations.CommitStatus.FAILURE) {
+ throw new CommitFailedException(
+ e, "The table %s.%s has been modified concurrently", database, tableName);
+ }
+ } else {
+ LOG.error(
+ "Cannot tell if commit to {}.{} succeeded, attempting to reconnect and check.",
+ database,
+ tableName,
+ e);
+ commitStatus =
Review Comment:
I updated to address this issue. At least I think the new `checkCommitStatusStrict` should not follow this pattern. @pvary please let me know if you think otherwise.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2013339525
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java:
##########
@@ -40,16 +40,10 @@ private HiveMetastoreExtension(String databaseName, Map<String, String> hiveConf
@Override
public void beforeAll(ExtensionContext extensionContext) throws Exception {
- metastore = new TestHiveMetastore();
- HiveConf hiveConfWithOverrides = new HiveConf(TestHiveMetastore.class);
- if (hiveConfOverride != null) {
- for (Map.Entry<String, String> kv : hiveConfOverride.entrySet()) {
- hiveConfWithOverrides.set(kv.getKey(), kv.getValue());
- }
- }
+ metastore = new TestHiveMetastore(hiveConfOverride);
- metastore.start(hiveConfWithOverrides);
- metastoreClient = new HiveMetaStoreClient(hiveConfWithOverrides);
+ metastore.start(new HiveConf(TestHiveMetastore.class));
+ metastoreClient = new HiveMetaStoreClient(metastore.hiveConf());
Review Comment:
`METASTORE_TRY_DIRECT_SQL` is hard coded to false in `TestHiveMetastore::initConf`. This change is to make it possible to override the config in our test.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2020535939
##########
hive-metastore/src/test/resources/hive-schema-3.1.0.derby.sql:
##########
@@ -130,7 +130,7 @@ CREATE TABLE "APP"."TAB_COL_STATS"(
"BIT_VECTOR" BLOB
);
-CREATE TABLE "APP"."TABLE_PARAMS" ("TBL_ID" BIGINT NOT NULL, "PARAM_KEY" VARCHAR(256) NOT NULL, "PARAM_VALUE" CLOB);
Review Comment:
FYI CLOB handling was fixed in https://github.com/apache/hive/pull/5386/files#diff-bcca13f6cc251df321e8fe80568ef0334a1d44f7e5e7ff2fcaa06ab4f05bbdf9R3387
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary merged PR #12637:
URL: https://github.com/apache/iceberg/pull/12637
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2011702700
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties, commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
Review Comment:
Why did you decide to create a new 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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2020560062
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -287,15 +277,33 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
e);
}
- LOG.error(
- "Cannot tell if commit to {}.{} succeeded, attempting to reconnect and check.",
- database,
- tableName,
- e);
commitStatus = BaseMetastoreOperations.CommitStatus.UNKNOWN;
- commitStatus =
- BaseMetastoreOperations.CommitStatus.valueOf(
- checkCommitStatus(newMetadataLocation, metadata).name());
+ if (e.getMessage() != null
+ && e.getMessage()
+ .contains(
+ "The table has been modified. The parameter value for key '"
+ + HiveTableOperations.METADATA_LOCATION_PROP
+ + "' is")) {
+ // It's possible the HMS client incorrectly retries a successful operation, due to network
+ // issue for example, and triggers this exception. So we need double-check to make sure
+ // this is really a concurrent modification. Hitting this exception means no pending
+ // requests, if any, can succeed later, so it's safe to check status in strict mode
+ commitStatus = checkCommitStatusStrict(newMetadataLocation, metadata);
+ if (commitStatus == BaseMetastoreOperations.CommitStatus.FAILURE) {
+ throw new CommitFailedException(
+ e, "The table %s.%s has been modified concurrently", database, tableName);
+ }
+ } else {
+ LOG.error(
+ "Cannot tell if commit to {}.{} succeeded, attempting to reconnect and check.",
+ database,
+ tableName,
+ e);
+ commitStatus =
Review Comment:
why do we need this weird casting? Note, it's skipped when we don't hit the else branch
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2011842134
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties, commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
Review Comment:
Reading the case in #11814 we need to identify the case when the previous attempt to commit was succeeded, and we don't remove the metadata files in this case. Otherwise we can't be sure of anything, and we should return UNKNOWN.
Is my reasoning correct?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2013487458
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java:
##########
@@ -40,16 +40,10 @@ private HiveMetastoreExtension(String databaseName, Map<String, String> hiveConf
@Override
public void beforeAll(ExtensionContext extensionContext) throws Exception {
- metastore = new TestHiveMetastore();
- HiveConf hiveConfWithOverrides = new HiveConf(TestHiveMetastore.class);
- if (hiveConfOverride != null) {
- for (Map.Entry<String, String> kv : hiveConfOverride.entrySet()) {
- hiveConfWithOverrides.set(kv.getKey(), kv.getValue());
- }
- }
+ metastore = new TestHiveMetastore(hiveConfOverride);
- metastore.start(hiveConfWithOverrides);
- metastoreClient = new HiveMetaStoreClient(hiveConfWithOverrides);
+ metastore.start(new HiveConf(TestHiveMetastore.class));
+ metastoreClient = new HiveMetaStoreClient(metastore.hiveConf());
Review Comment:
OK I'll try setting the default value to true, if everything is fine, I'll revert this change
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2013442916
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##########
@@ -310,7 +310,7 @@ protected CommitStatus checkCommitStatus(String newMetadataLocation, TableMetada
* @return true if the new metadata location is the current metadata location or present within
* previous metadata files.
*/
- private boolean checkCurrentMetadataLocation(String newMetadataLocation) {
+ protected boolean checkCurrentMetadataLocation(String newMetadataLocation) {
Review Comment:
Added checkCommitStatusStrict instead. It seems more consistent with current APIs. Thanks~
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2013804651
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -98,11 +136,7 @@ protected CommitStatus checkCommitStatus(
newMetadataLocation);
status.set(CommitStatus.SUCCESS);
} else {
- LOG.warn(
- "Commit status check: Commit to {} of {} unknown, new metadata location is not current "
- + "or in history",
- tableOrViewName,
- newMetadataLocation);
Review Comment:
Keep this, it is still useful
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2013799362
##########
hive-metastore/src/test/resources/hive-schema-3.1.0.derby.sql:
##########
@@ -130,7 +130,7 @@ CREATE TABLE "APP"."TAB_COL_STATS"(
"BIT_VECTOR" BLOB
);
-CREATE TABLE "APP"."TABLE_PARAMS" ("TBL_ID" BIGINT NOT NULL, "PARAM_KEY" VARCHAR(256) NOT NULL, "PARAM_VALUE" CLOB);
Review Comment:
Do we want to change the other `CLOB`s in this file as well?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2012174241
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties, commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
Review Comment:
So to summarize when we get a "The table has been modified. The parameter value for key [..]" exception, then one of the following happened:
1. Someone did a concurrent commit - we need to return `CommitStatus.FAILURE`, so the commit could be retried above the new commit
2. The previous attempt of the commit has already happened - we need to return `CommitStatus.SUCCESS`, so no retries will happen, and there is no retries
3. We can't reach the HMS - we need to return `CommitStatus.UNKNOWN`
Sounds reasonable. Thanks for all the details!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2013335765
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties, commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
+ }
+
+ /**
+ * Attempt to load the content and see if any current or past metadata location matches the one we
+ * were attempting to set.
+ *
+ * @param tableOrViewName full name of the Table/View
+ * @param newMetadataLocation the path of the new commit file
+ * @param properties properties for retry
+ * @param commitStatusSupplier check if the latest metadata presents or not using metadata
+ * location for table.
+ * @return Empty if locations cannot be checked, e.g. unable to refresh. True if the new location
+ * is committed, false otherwise.
+ */
+ protected Optional<Boolean> metadataLocationCommitted(
Review Comment:
Yeah but I would like `checkCommitStatusStrict` and `checkCommitStatus` to share the same underlying check logic. The difference is `checkCommitStatusStrict` treats false as `CommitStatus.FAILURE` while `checkCommitStatus` treats false as `CommitStatus.UNKNOWN`. So I think we can keep the method returning `Optional<Boolean>`, but make it private?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2011840617
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties, commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
Review Comment:
Hmm interesting... But I think we don't have to worry about that in our case, because the double check happens after HMS explicitly returns an exception, which means it has finished processing 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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2012235572
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##########
@@ -310,7 +310,7 @@ protected CommitStatus checkCommitStatus(String newMetadataLocation, TableMetada
* @return true if the new metadata location is the current metadata location or present within
* previous metadata files.
*/
- private boolean checkCurrentMetadataLocation(String newMetadataLocation) {
+ protected boolean checkCurrentMetadataLocation(String newMetadataLocation) {
Review Comment:
I'm not sure which is better:
1. Making this method `protected`, or
2. Creating a new `checkCommitStatusStrict(String newMetadataLocation, TableMetadata config)` protected method
WDYT?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2011857349
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties, commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
Review Comment:
I don't think we can return UNKNOWN if the metadata location is not found. Because UNKNOWN leads to a CommitStateUnknownException and it won't be retried, which basically means concurrent commit is not possible with NoLock mode.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2013798052
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java:
##########
@@ -40,16 +40,10 @@ private HiveMetastoreExtension(String databaseName, Map<String, String> hiveConf
@Override
public void beforeAll(ExtensionContext extensionContext) throws Exception {
- metastore = new TestHiveMetastore();
- HiveConf hiveConfWithOverrides = new HiveConf(TestHiveMetastore.class);
- if (hiveConfOverride != null) {
- for (Map.Entry<String, String> kv : hiveConfOverride.entrySet()) {
- hiveConfWithOverrides.set(kv.getKey(), kv.getValue());
- }
- }
+ metastore = new TestHiveMetastore(hiveConfOverride);
- metastore.start(hiveConfWithOverrides);
- metastoreClient = new HiveMetaStoreClient(hiveConfWithOverrides);
+ metastore.start(new HiveConf(TestHiveMetastore.class));
+ metastoreClient = new HiveMetaStoreClient(metastore.hiveConf());
Review Comment:
I think the Spark issue still persists when the directSql is turned off, so we don't want to change that before talking to the owners of the test.
Just create this method, and the old methods should use this:
```
/**
* Starts a TestHiveMetastore with a provided connection pool size and HiveConf.
*
* @param conf The hive configuration to use
* @param poolSize The number of threads in the executor pool
* @param directSql Used to turn on directSql
*/
public void start(HiveConf conf, int poolSize, boolean directSql) {
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2011727128
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -287,15 +277,25 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
e);
}
- LOG.error(
- "Cannot tell if commit to {}.{} succeeded, attempting to reconnect and check.",
- database,
- tableName,
- e);
- commitStatus = BaseMetastoreOperations.CommitStatus.UNKNOWN;
- commitStatus =
- BaseMetastoreOperations.CommitStatus.valueOf(
- checkCommitStatus(newMetadataLocation, metadata).name());
+ if (e.getMessage() != null
+ && e.getMessage()
+ .contains(
+ "The table has been modified. The parameter value for key '"
+ + HiveTableOperations.METADATA_LOCATION_PROP
+ + "' is")) {
+ commitStatus = handleConcurrentModification(e, newMetadataLocation, metadata);
Review Comment:
Is it better to call this `handlePossibleConcurrentModification`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2011862799
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties, commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
Review Comment:
> Hmm interesting... But I think we don't have to worry about that in our case, because the double check happens after HMS explicitly returns an exception, which means it has finished processing right?
There could me multiple HMS instances, and another one might still working on serving the previous request
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#issuecomment-2753884603
Thanks @lirui-apache for all your work on this!
Last changes and we are golden.
We can ping Russell when we are ready, but if he doesn't have time, then I will be happy to merge it later this week.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2013798052
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java:
##########
@@ -40,16 +40,10 @@ private HiveMetastoreExtension(String databaseName, Map<String, String> hiveConf
@Override
public void beforeAll(ExtensionContext extensionContext) throws Exception {
- metastore = new TestHiveMetastore();
- HiveConf hiveConfWithOverrides = new HiveConf(TestHiveMetastore.class);
- if (hiveConfOverride != null) {
- for (Map.Entry<String, String> kv : hiveConfOverride.entrySet()) {
- hiveConfWithOverrides.set(kv.getKey(), kv.getValue());
- }
- }
+ metastore = new TestHiveMetastore(hiveConfOverride);
- metastore.start(hiveConfWithOverrides);
- metastoreClient = new HiveMetaStoreClient(hiveConfWithOverrides);
+ metastore.start(new HiveConf(TestHiveMetastore.class));
+ metastoreClient = new HiveMetaStoreClient(metastore.hiveConf());
Review Comment:
I think the Spark issue still persists when the directSql is turned off, so we don't want to change that before talking to the owners of the test.
Just create this method, and the old methods should use this:
```
/**
* Starts a TestHiveMetastore with a provided connection pool size and HiveConf.
*
* @param conf The hive configuration to use
* @param poolSize The number of threads in the executor pool
* @param directSql Used to turn on directSql
*/
public void start(HiveConf conf, int poolSize, boolean directSql) {
[..]
initConf(conf, port, directSql);
[..]
}
/**
* Starts a TestHiveMetastore with a provided connection pool size and HiveConf.
*
* @param conf The hive configuration to use
* @param poolSize The number of threads in the executor pool
*/
public void start(HiveConf conf, int poolSize) {
start(conf, poolSize, false);
}
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2013977588
##########
hive-metastore/src/test/resources/hive-schema-3.1.0.derby.sql:
##########
@@ -130,7 +130,7 @@ CREATE TABLE "APP"."TAB_COL_STATS"(
"BIT_VECTOR" BLOB
);
-CREATE TABLE "APP"."TABLE_PARAMS" ("TBL_ID" BIGINT NOT NULL, "PARAM_KEY" VARCHAR(256) NOT NULL, "PARAM_VALUE" CLOB);
Review Comment:
Add more changes to be consistent with HIVE-16667 and HIVE-25574
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#issuecomment-2774555793
Thanks to @pvary and @deniskuzZ for reviewing and merging the 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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2011760337
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -324,6 +324,25 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
"Committed to table {} with the new metadata location {}", fullName, newMetadataLocation);
}
+ private BaseMetastoreOperations.CommitStatus handleConcurrentModification(
+ Throwable throwable, String newMetadataLocation, TableMetadata metadata) {
Review Comment:
What is the difference between the `checkCommitStatus` and the `handleConcurrentModification`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2011871910
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties, commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
Review Comment:
> >Hmm interesting... But I think we don't have to worry about that in our case, because the double check happens after HMS explicitly returns an exception, which means it has finished processing right?
> There could me multiple HMS instances, and another one might still working on serving the previous request
But that will fail... Let me think a bit about this.
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties, commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
Review Comment:
> > Hmm interesting... But I think we don't have to worry about that in our case, because the double check happens after HMS explicitly returns an exception, which means it has finished processing right?
> There could me multiple HMS instances, and another one might still working on serving the previous request
But that will fail... Let me think a bit about 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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2011741211
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -287,15 +277,25 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
e);
}
- LOG.error(
- "Cannot tell if commit to {}.{} succeeded, attempting to reconnect and check.",
- database,
- tableName,
- e);
- commitStatus = BaseMetastoreOperations.CommitStatus.UNKNOWN;
- commitStatus =
- BaseMetastoreOperations.CommitStatus.valueOf(
- checkCommitStatus(newMetadataLocation, metadata).name());
+ if (e.getMessage() != null
+ && e.getMessage()
Review Comment:
Could we have a same issue here than with #11576?
If there is a new exception during the `handleConcurrentModification` call, then the commit status is unset, and the metadata will be removed.
Maybe leave the
```
commitStatus = BaseMetastoreOperations.CommitStatus.UNKNOWN;
```
line before the `if` statement?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2011825411
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties, commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
Review Comment:
I have checked the history, and found that the `CommitStatus.FAILURE` is not returned because of this PR: #3717.
It says that we can't be sure about a failure as the HMS might still processing our request.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2012262765
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java:
##########
@@ -40,16 +40,10 @@ private HiveMetastoreExtension(String databaseName, Map<String, String> hiveConf
@Override
public void beforeAll(ExtensionContext extensionContext) throws Exception {
- metastore = new TestHiveMetastore();
- HiveConf hiveConfWithOverrides = new HiveConf(TestHiveMetastore.class);
- if (hiveConfOverride != null) {
- for (Map.Entry<String, String> kv : hiveConfOverride.entrySet()) {
- hiveConfWithOverrides.set(kv.getKey(), kv.getValue());
- }
- }
+ metastore = new TestHiveMetastore(hiveConfOverride);
- metastore.start(hiveConfWithOverrides);
- metastoreClient = new HiveMetaStoreClient(hiveConfWithOverrides);
+ metastore.start(new HiveConf(TestHiveMetastore.class));
+ metastoreClient = new HiveMetaStoreClient(metastore.hiveConf());
Review Comment:
Why is this change?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2013478387
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties, commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
+ }
+
+ /**
+ * Attempt to load the content and see if any current or past metadata location matches the one we
+ * were attempting to set.
+ *
+ * @param tableOrViewName full name of the Table/View
+ * @param newMetadataLocation the path of the new commit file
+ * @param properties properties for retry
+ * @param commitStatusSupplier check if the latest metadata presents or not using metadata
+ * location for table.
+ * @return Empty if locations cannot be checked, e.g. unable to refresh. True if the new location
+ * is committed, false otherwise.
+ */
+ protected Optional<Boolean> metadataLocationCommitted(
Review Comment:
Or could we just call the `checkCommitStatusStrict` inside the `checkCommitStatus` and reinterpret the result?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2011742223
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -287,15 +277,25 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
e);
}
- LOG.error(
- "Cannot tell if commit to {}.{} succeeded, attempting to reconnect and check.",
- database,
- tableName,
- e);
- commitStatus = BaseMetastoreOperations.CommitStatus.UNKNOWN;
- commitStatus =
- BaseMetastoreOperations.CommitStatus.valueOf(
- checkCommitStatus(newMetadataLocation, metadata).name());
+ if (e.getMessage() != null
+ && e.getMessage()
Review Comment:
Also, we could add a comment there. It took me a while the understand the reasoning behind it 😄
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#issuecomment-2750270432
I need to make these changes to enable a test with NoLock:
1. Allow direct SQL in test metastore, because the transactional altering table now relies on direct SQL as of [HIVE-28121](https://issues.apache.org/jira/browse/HIVE-28121)
2. Change the type of `TABLE_PARAMS.PARAM_VALUE` from `CLOB` to `VARCHAR(32672)`. This is because derby doesn't allow comparing clob type with a string, so the direct SQL will fail. I believe tests in Hive can run because Hive initializes the underlying DB with the [JDO mapping](https://github.com/apache/hive/blob/rel/release-2.3.10/metastore/src/model/package.jdo#L173) instead of the metastore SQL script.
If these changes turn out to be unacceptable, I suppose we can just mock the exception in the HMS client.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2013804651
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -98,11 +136,7 @@ protected CommitStatus checkCommitStatus(
newMetadataLocation);
status.set(CommitStatus.SUCCESS);
} else {
- LOG.warn(
- "Commit status check: Commit to {} of {} unknown, new metadata location is not current "
- + "or in history",
- tableOrViewName,
- newMetadataLocation);
Review Comment:
Keep this, it is still useful
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2013782944
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java:
##########
@@ -40,16 +40,10 @@ private HiveMetastoreExtension(String databaseName, Map<String, String> hiveConf
@Override
public void beforeAll(ExtensionContext extensionContext) throws Exception {
- metastore = new TestHiveMetastore();
- HiveConf hiveConfWithOverrides = new HiveConf(TestHiveMetastore.class);
- if (hiveConfOverride != null) {
- for (Map.Entry<String, String> kv : hiveConfOverride.entrySet()) {
- hiveConfWithOverrides.set(kv.getKey(), kv.getValue());
- }
- }
+ metastore = new TestHiveMetastore(hiveConfOverride);
- metastore.start(hiveConfWithOverrides);
- metastoreClient = new HiveMetaStoreClient(hiveConfWithOverrides);
+ metastore.start(new HiveConf(TestHiveMetastore.class));
+ metastoreClient = new HiveMetaStoreClient(metastore.hiveConf());
Review Comment:
Enabling direct SQL globally seems to fix the [weird test case](https://github.com/apache/iceberg/blob/af99da6c2daa8e53876638c839b0f729f2f3a86e/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java#L741). I did some debug and believe it's because direct SQL gets the partition filter pushdown correct and retrieves the corresponding partitions. Should I update the test cases here, or leave it to another 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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#issuecomment-2754356345
Looks good to me.
@RussellSpitzer: Do you want to review this before merge?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2012254481
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java:
##########
@@ -40,16 +40,10 @@ private HiveMetastoreExtension(String databaseName, Map<String, String> hiveConf
@Override
public void beforeAll(ExtensionContext extensionContext) throws Exception {
- metastore = new TestHiveMetastore();
- HiveConf hiveConfWithOverrides = new HiveConf(TestHiveMetastore.class);
- if (hiveConfOverride != null) {
- for (Map.Entry<String, String> kv : hiveConfOverride.entrySet()) {
- hiveConfWithOverrides.set(kv.getKey(), kv.getValue());
- }
- }
+ metastore = new TestHiveMetastore(hiveConfOverride);
- metastore.start(hiveConfWithOverrides);
- metastoreClient = new HiveMetaStoreClient(hiveConfWithOverrides);
+ metastore.start(new HiveConf(TestHiveMetastore.class));
+ metastoreClient = new HiveMetaStoreClient(metastore.hiveConf());
Review Comment:
Why do we add this change?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2012254481
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java:
##########
@@ -40,16 +40,10 @@ private HiveMetastoreExtension(String databaseName, Map<String, String> hiveConf
@Override
public void beforeAll(ExtensionContext extensionContext) throws Exception {
- metastore = new TestHiveMetastore();
- HiveConf hiveConfWithOverrides = new HiveConf(TestHiveMetastore.class);
- if (hiveConfOverride != null) {
- for (Map.Entry<String, String> kv : hiveConfOverride.entrySet()) {
- hiveConfWithOverrides.set(kv.getKey(), kv.getValue());
- }
- }
+ metastore = new TestHiveMetastore(hiveConfOverride);
- metastore.start(hiveConfWithOverrides);
- metastoreClient = new HiveMetaStoreClient(hiveConfWithOverrides);
+ metastore.start(new HiveConf(TestHiveMetastore.class));
+ metastoreClient = new HiveMetaStoreClient(metastore.hiveConf());
Review Comment:
Why do we add this change?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2013480471
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java:
##########
@@ -40,16 +40,10 @@ private HiveMetastoreExtension(String databaseName, Map<String, String> hiveConf
@Override
public void beforeAll(ExtensionContext extensionContext) throws Exception {
- metastore = new TestHiveMetastore();
- HiveConf hiveConfWithOverrides = new HiveConf(TestHiveMetastore.class);
- if (hiveConfOverride != null) {
- for (Map.Entry<String, String> kv : hiveConfOverride.entrySet()) {
- hiveConfWithOverrides.set(kv.getKey(), kv.getValue());
- }
- }
+ metastore = new TestHiveMetastore(hiveConfOverride);
- metastore.start(hiveConfWithOverrides);
- metastoreClient = new HiveMetaStoreClient(hiveConfWithOverrides);
+ metastore.start(new HiveConf(TestHiveMetastore.class));
+ metastoreClient = new HiveMetaStoreClient(metastore.hiveConf());
Review Comment:
Could you please try to set this globally, and see if there is an issue with it? This is just test code, and I would like to try to keep it as simple as possible.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Hive: Double check commit status in case of commit conflict for NoLock [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#issuecomment-2772423961
Thanks for the PR @lirui-apache!
Thanks for the review @deniskuzZ!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org