From 98a9b73cfea46a2cd142000b2bf7ce62f1ac5304 Mon Sep 17 00:00:00 2001 From: yufeigu Date: Mon, 21 Mar 2022 15:59:38 -0700 Subject: [PATCH 01/13] Core: Add Reserved Snapshot Stats Table Property and Expose in HMS. --- .../org/apache/iceberg/TableProperties.java | 34 ++++++++++++- .../iceberg/hive/HiveTableOperations.java | 20 ++++++++ .../apache/iceberg/hive/TestHiveCatalog.java | 51 +++++++++++++++++++ 3 files changed, 104 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/iceberg/TableProperties.java b/core/src/main/java/org/apache/iceberg/TableProperties.java index ee25d9097bec..0005fa8e04aa 100644 --- a/core/src/main/java/org/apache/iceberg/TableProperties.java +++ b/core/src/main/java/org/apache/iceberg/TableProperties.java @@ -47,6 +47,34 @@ private TableProperties() { */ public static final String UUID = "uuid"; + /** + * Reserved table property for the total number of snapshots. + *

+ * This reserved property is used to store the total number of snapshots. + */ + public static final String SNAPSHOT_COUNT = "snapshot-count"; + + /** + * Reserved table property for current snapshot summary. + *

+ * This reserved property is used to store the current snapshot summary. + */ + public static final String CURRENT_SNAPSHOT_SUMMARY = "current-snapshot-summary"; + + /** + * Reserved table property for current snapshot id. + *

+ * This reserved property is used to store the current snapshot id. + */ + public static final String CURRENT_SNAPSHOT_ID = "current-snapshot-id"; + + /** + * Reserved table property for current snapshot timestamp. + *

+ * This reserved property is used to store the current snapshot timestamp. + */ + public static final String CURRENT_SNAPSHOT_TIMESTAMP = "current-snapshot-timestamp-ms"; + /** * Reserved Iceberg table properties list. *

@@ -55,7 +83,11 @@ private TableProperties() { */ public static final Set RESERVED_PROPERTIES = ImmutableSet.of( FORMAT_VERSION, - UUID + UUID, + SNAPSHOT_COUNT, + CURRENT_SNAPSHOT_ID, + CURRENT_SNAPSHOT_SUMMARY, + CURRENT_SNAPSHOT_TIMESTAMP ); public static final String COMMIT_NUM_RETRIES = "commit.retry.num-retries"; diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java index c6d081f731bc..7fa70cc59332 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java @@ -19,6 +19,7 @@ package org.apache.iceberg.hive; +import com.fasterxml.jackson.core.JsonProcessingException; import com.github.benmanes.caffeine.cache.Cache; import com.github.benmanes.caffeine.cache.Caffeine; import java.net.InetAddress; @@ -71,6 +72,7 @@ import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.util.JsonUtil; import org.apache.iceberg.util.Tasks; import org.apache.thrift.TException; import org.slf4j.Logger; @@ -402,9 +404,27 @@ private void setHmsTableParameters(String newMetadataLocation, Table tbl, TableM parameters.put(StatsSetupConst.TOTAL_SIZE, summary.get(SnapshotSummary.TOTAL_FILE_SIZE_PROP)); } + setSnapshotStats(metadata, parameters); + tbl.setParameters(parameters); } + private void setSnapshotStats(TableMetadata metadata, Map parameters) { + Snapshot currentSnapshot = metadata.currentSnapshot(); + if (currentSnapshot != null) { + parameters.put(TableProperties.CURRENT_SNAPSHOT_ID, String.valueOf(currentSnapshot.snapshotId())); + parameters.put(TableProperties.CURRENT_SNAPSHOT_TIMESTAMP, String.valueOf(currentSnapshot.timestampMillis())); + try { + parameters.put(TableProperties.CURRENT_SNAPSHOT_SUMMARY, + JsonUtil.mapper().writeValueAsString(currentSnapshot.summary())); + } catch (JsonProcessingException e) { + LOG.warn("Failed to convert snapshot summary to a json string", e); + } + } + + parameters.put(TableProperties.SNAPSHOT_COUNT, String.valueOf(metadata.snapshots().size())); + } + private StorageDescriptor storageDescriptor(TableMetadata metadata, boolean hiveEngineEnabled) { final StorageDescriptor storageDescriptor = new StorageDescriptor(); diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 74716b771cb0..991c0c9bd09f 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -21,9 +21,13 @@ import java.util.List; import java.util.Map; +import java.util.UUID; import org.apache.hadoop.hive.metastore.api.Database; import org.apache.iceberg.AssertHelpers; import org.apache.iceberg.CachingCatalog; +import org.apache.iceberg.DataFile; +import org.apache.iceberg.DataFiles; +import org.apache.iceberg.FileFormat; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.Schema; import org.apache.iceberg.SortOrder; @@ -468,4 +472,51 @@ public void testUUIDinTableProperties() throws Exception { catalog.dropTable(tableIdentifier); } } + + @Test + public void testSnapshotStatsTableProperties() throws Exception { + Schema schema = new Schema( + required(1, "id", Types.IntegerType.get(), "unique ID"), + required(2, "data", Types.StringType.get()) + ); + TableIdentifier tableIdentifier = TableIdentifier.of(DB_NAME, "tbl"); + String location = temp.newFolder("tbl").toString(); + + try { + catalog.buildTable(tableIdentifier, schema) + .withLocation(location) + .create(); + + String tableName = tableIdentifier.name(); + org.apache.hadoop.hive.metastore.api.Table hmsTable = + metastoreClient.getTable(tableIdentifier.namespace().level(0), tableName); + + // check whether parameters are in expected state + Map parameters = hmsTable.getParameters(); + Assert.assertEquals("0", parameters.get(TableProperties.SNAPSHOT_COUNT)); + Assert.assertNull(parameters.get(TableProperties.CURRENT_SNAPSHOT_SUMMARY)); + Assert.assertNull(parameters.get(TableProperties.CURRENT_SNAPSHOT_ID)); + Assert.assertNull(parameters.get(TableProperties.CURRENT_SNAPSHOT_TIMESTAMP)); + + // create a snapshot + Table icebergTable = catalog.loadTable(tableIdentifier); + String fileName = UUID.randomUUID().toString(); + DataFile file = DataFiles.builder(icebergTable.spec()) + .withPath(FileFormat.PARQUET.addExtension(fileName)) + .withRecordCount(2) + .withFileSizeInBytes(0) + .build(); + icebergTable.newFastAppend().appendFile(file).commit(); + + // check whether parameters are in expected state + hmsTable = metastoreClient.getTable(tableIdentifier.namespace().level(0), tableName); + parameters = hmsTable.getParameters(); + Assert.assertEquals("1", parameters.get(TableProperties.SNAPSHOT_COUNT)); + Assert.assertNotNull(parameters.get(TableProperties.CURRENT_SNAPSHOT_SUMMARY)); + Assert.assertNotNull(parameters.get(TableProperties.CURRENT_SNAPSHOT_ID)); + Assert.assertNotNull(parameters.get(TableProperties.CURRENT_SNAPSHOT_TIMESTAMP)); + } finally { + catalog.dropTable(tableIdentifier); + } + } } From 5fe283c9dfbbaf5d38c2136f1adfaa86e0f090f7 Mon Sep 17 00:00:00 2001 From: yufeigu Date: Thu, 31 Mar 2022 11:56:42 -0700 Subject: [PATCH 02/13] Fix the unit test failure --- .../iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java b/mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java index 938ba1997b28..e588c0b3ac79 100644 --- a/mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java +++ b/mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java @@ -624,7 +624,7 @@ public void testIcebergAndHmsTableProperties() throws Exception { Assert.assertEquals(expectedIcebergProperties, icebergTable.properties()); if (Catalogs.hiveCatalog(shell.getHiveConf(), tableProperties)) { - Assert.assertEquals(11, hmsParams.size()); + Assert.assertEquals(12, hmsParams.size()); Assert.assertEquals("initial_val", hmsParams.get("custom_property")); Assert.assertEquals("TRUE", hmsParams.get(InputFormatConfig.EXTERNAL_TABLE_PURGE)); Assert.assertEquals("TRUE", hmsParams.get("EXTERNAL")); @@ -662,7 +662,7 @@ public void testIcebergAndHmsTableProperties() throws Exception { .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); if (Catalogs.hiveCatalog(shell.getHiveConf(), tableProperties)) { - Assert.assertEquals(14, hmsParams.size()); // 2 newly-added properties + previous_metadata_location prop + Assert.assertEquals(15, hmsParams.size()); // 2 newly-added properties + previous_metadata_location prop Assert.assertEquals("true", hmsParams.get("new_prop_1")); Assert.assertEquals("false", hmsParams.get("new_prop_2")); Assert.assertEquals("new_val", hmsParams.get("custom_property")); From c7b0e624ec6b0709909ff7e61720c27c7ccd4e20 Mon Sep 17 00:00:00 2001 From: yufeigu Date: Sun, 3 Apr 2022 18:02:31 -0700 Subject: [PATCH 03/13] Doesn't expose the summary if its length is more than 4000 chars. --- .../org/apache/iceberg/hive/HiveTableOperations.java | 9 ++++++--- .../java/org/apache/iceberg/hive/TestHiveCatalog.java | 10 +++++++--- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java index 7fa70cc59332..94473f71a1a5 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java @@ -67,6 +67,7 @@ import org.apache.iceberg.hadoop.ConfigProperties; import org.apache.iceberg.io.FileIO; import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.relocated.com.google.common.collect.BiMap; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableBiMap; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; @@ -415,10 +416,12 @@ private void setSnapshotStats(TableMetadata metadata, Map parame parameters.put(TableProperties.CURRENT_SNAPSHOT_ID, String.valueOf(currentSnapshot.snapshotId())); parameters.put(TableProperties.CURRENT_SNAPSHOT_TIMESTAMP, String.valueOf(currentSnapshot.timestampMillis())); try { - parameters.put(TableProperties.CURRENT_SNAPSHOT_SUMMARY, - JsonUtil.mapper().writeValueAsString(currentSnapshot.summary())); + String summary = JsonUtil.mapper().writeValueAsString(currentSnapshot.summary()); + Preconditions.checkArgument(summary.length() <= 4000, + "Failed to expose the current snapshot summary in HMS since it exceeds 4000 characters"); + parameters.put(TableProperties.CURRENT_SNAPSHOT_SUMMARY, summary); } catch (JsonProcessingException e) { - LOG.warn("Failed to convert snapshot summary to a json string", e); + LOG.warn("Failed to convert current snapshot summary to a json string", e); } } diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 991c0c9bd09f..0e33663001fb 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -46,6 +46,7 @@ import org.apache.iceberg.transforms.Transform; import org.apache.iceberg.transforms.Transforms; import org.apache.iceberg.types.Types; +import org.apache.iceberg.util.JsonUtil; import org.apache.thrift.TException; import org.junit.Assert; import org.junit.Rule; @@ -512,9 +513,12 @@ public void testSnapshotStatsTableProperties() throws Exception { hmsTable = metastoreClient.getTable(tableIdentifier.namespace().level(0), tableName); parameters = hmsTable.getParameters(); Assert.assertEquals("1", parameters.get(TableProperties.SNAPSHOT_COUNT)); - Assert.assertNotNull(parameters.get(TableProperties.CURRENT_SNAPSHOT_SUMMARY)); - Assert.assertNotNull(parameters.get(TableProperties.CURRENT_SNAPSHOT_ID)); - Assert.assertNotNull(parameters.get(TableProperties.CURRENT_SNAPSHOT_TIMESTAMP)); + String summary = JsonUtil.mapper().writeValueAsString(icebergTable.currentSnapshot().summary()); + Assert.assertEquals(summary, parameters.get(TableProperties.CURRENT_SNAPSHOT_SUMMARY)); + long snapshotId = icebergTable.currentSnapshot().snapshotId(); + Assert.assertEquals(String.valueOf(snapshotId), parameters.get(TableProperties.CURRENT_SNAPSHOT_ID)); + Assert.assertEquals(String.valueOf(icebergTable.currentSnapshot().timestampMillis()), + parameters.get(TableProperties.CURRENT_SNAPSHOT_TIMESTAMP)); } finally { catalog.dropTable(tableIdentifier); } From 1d96a1b4852d73b75ed4b4b42b8cc698a513b021 Mon Sep 17 00:00:00 2001 From: yufeigu Date: Sun, 3 Apr 2022 18:16:21 -0700 Subject: [PATCH 04/13] Doesn't expose the summary if its length is more than 4000 chars. --- .../apache/iceberg/hive/HiveTableOperations.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java index 94473f71a1a5..6181342a462b 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java @@ -67,7 +67,6 @@ import org.apache.iceberg.hadoop.ConfigProperties; import org.apache.iceberg.io.FileIO; import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting; -import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.relocated.com.google.common.collect.BiMap; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableBiMap; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; @@ -93,6 +92,7 @@ public class HiveTableOperations extends BaseMetastoreTableOperations { private static final String HIVE_LOCK_CHECK_MAX_WAIT_MS = "iceberg.hive.lock-check-max-wait-ms"; private static final String HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES = "iceberg.hive.metadata-refresh-max-retries"; private static final String HIVE_TABLE_LEVEL_LOCK_EVICT_MS = "iceberg.hive.table-level-lock-evict-ms"; + private static final long HIVE_TABLE_PROPERTY_VALUE_SIZE_MAX = 4000; private static final long HIVE_ACQUIRE_LOCK_TIMEOUT_MS_DEFAULT = 3 * 60 * 1000; // 3 minutes private static final long HIVE_LOCK_CHECK_MIN_WAIT_MS_DEFAULT = 50; // 50 milliseconds private static final long HIVE_LOCK_CHECK_MAX_WAIT_MS_DEFAULT = 5 * 1000; // 5 seconds @@ -417,11 +417,14 @@ private void setSnapshotStats(TableMetadata metadata, Map parame parameters.put(TableProperties.CURRENT_SNAPSHOT_TIMESTAMP, String.valueOf(currentSnapshot.timestampMillis())); try { String summary = JsonUtil.mapper().writeValueAsString(currentSnapshot.summary()); - Preconditions.checkArgument(summary.length() <= 4000, - "Failed to expose the current snapshot summary in HMS since it exceeds 4000 characters"); - parameters.put(TableProperties.CURRENT_SNAPSHOT_SUMMARY, summary); + if (summary.length() <= HIVE_TABLE_PROPERTY_VALUE_SIZE_MAX) { + parameters.put(TableProperties.CURRENT_SNAPSHOT_SUMMARY, summary); + } else { + LOG.warn("Not expose the current snapshot({}) summary in HMS since it exceeds {} characters", + currentSnapshot.snapshotId(), HIVE_TABLE_PROPERTY_VALUE_SIZE_MAX); + } } catch (JsonProcessingException e) { - LOG.warn("Failed to convert current snapshot summary to a json string", e); + LOG.warn("Failed to convert current snapshot({}) summary to a json string", currentSnapshot.snapshotId(), e); } } From 8eac228561f547c633fc727d0e8511ca4b61b19d Mon Sep 17 00:00:00 2001 From: yufeigu Date: Wed, 6 Apr 2022 12:34:53 -0700 Subject: [PATCH 05/13] Resolve comments --- .../iceberg/hive/HiveTableOperations.java | 40 +++++++++----- .../apache/iceberg/hive/TestHiveCatalog.java | 54 +++++++++++++++++++ 2 files changed, 82 insertions(+), 12 deletions(-) diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java index 6181342a462b..283fa23ad0c9 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java @@ -92,7 +92,9 @@ public class HiveTableOperations extends BaseMetastoreTableOperations { private static final String HIVE_LOCK_CHECK_MAX_WAIT_MS = "iceberg.hive.lock-check-max-wait-ms"; private static final String HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES = "iceberg.hive.metadata-refresh-max-retries"; private static final String HIVE_TABLE_LEVEL_LOCK_EVICT_MS = "iceberg.hive.table-level-lock-evict-ms"; - private static final long HIVE_TABLE_PROPERTY_VALUE_SIZE_MAX = 4000; + private static final String HIVE_LEGACY_TABLE_PARAMETER_SIZE = "iceberg.hive.legacy.table.parameter.size"; + private static final long HIVE_TABLE_PARAMETER_SIZE_MAX_DEFAULT = 32672; + private static final long HIVE_TABLE_PARAMETER_SIZE_MAX_LEGACY = 4000; private static final long HIVE_ACQUIRE_LOCK_TIMEOUT_MS_DEFAULT = 3 * 60 * 1000; // 3 minutes private static final long HIVE_LOCK_CHECK_MIN_WAIT_MS_DEFAULT = 50; // 50 milliseconds private static final long HIVE_LOCK_CHECK_MAX_WAIT_MS_DEFAULT = 5 * 1000; // 5 seconds @@ -151,6 +153,7 @@ private static class WaitingForLockException extends RuntimeException { private final long lockAcquireTimeout; private final long lockCheckMinWaitTime; private final long lockCheckMaxWaitTime; + private final long maxHiveTableParameterSize; private final int metadataRefreshMaxRetries; private final FileIO fileIO; private final ClientPool metaClients; @@ -173,6 +176,8 @@ protected HiveTableOperations(Configuration conf, ClientPool metaClients, FileIO conf.getInt(HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES, HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES_DEFAULT); long tableLevelLockCacheEvictionTimeout = conf.getLong(HIVE_TABLE_LEVEL_LOCK_EVICT_MS, HIVE_TABLE_LEVEL_LOCK_EVICT_MS_DEFAULT); + this.maxHiveTableParameterSize = conf.getBoolean(HIVE_LEGACY_TABLE_PARAMETER_SIZE, false) ? + HIVE_TABLE_PARAMETER_SIZE_MAX_LEGACY : HIVE_TABLE_PARAMETER_SIZE_MAX_DEFAULT; initTableLevelLockCache(tableLevelLockCacheEvictionTimeout); } @@ -415,22 +420,33 @@ private void setSnapshotStats(TableMetadata metadata, Map parame if (currentSnapshot != null) { parameters.put(TableProperties.CURRENT_SNAPSHOT_ID, String.valueOf(currentSnapshot.snapshotId())); parameters.put(TableProperties.CURRENT_SNAPSHOT_TIMESTAMP, String.valueOf(currentSnapshot.timestampMillis())); - try { - String summary = JsonUtil.mapper().writeValueAsString(currentSnapshot.summary()); - if (summary.length() <= HIVE_TABLE_PROPERTY_VALUE_SIZE_MAX) { - parameters.put(TableProperties.CURRENT_SNAPSHOT_SUMMARY, summary); - } else { - LOG.warn("Not expose the current snapshot({}) summary in HMS since it exceeds {} characters", - currentSnapshot.snapshotId(), HIVE_TABLE_PROPERTY_VALUE_SIZE_MAX); - } - } catch (JsonProcessingException e) { - LOG.warn("Failed to convert current snapshot({}) summary to a json string", currentSnapshot.snapshotId(), e); - } + setSnapshotSummary(parameters, currentSnapshot); + } else { + parameters.remove(TableProperties.CURRENT_SNAPSHOT_ID); + parameters.remove(TableProperties.CURRENT_SNAPSHOT_TIMESTAMP); + parameters.remove(TableProperties.CURRENT_SNAPSHOT_SUMMARY); } parameters.put(TableProperties.SNAPSHOT_COUNT, String.valueOf(metadata.snapshots().size())); } + @VisibleForTesting + void setSnapshotSummary(Map parameters, Snapshot currentSnapshot) { + try { + String summary = JsonUtil.mapper().writeValueAsString(currentSnapshot.summary()); + if (summary.length() <= maxHiveTableParameterSize) { + parameters.put(TableProperties.CURRENT_SNAPSHOT_SUMMARY, summary); + } else { + parameters.remove(TableProperties.CURRENT_SNAPSHOT_SUMMARY); + LOG.warn("Not exposing the current snapshot({}) summary in HMS since it exceeds {} characters", + currentSnapshot.snapshotId(), maxHiveTableParameterSize); + } + } catch (JsonProcessingException e) { + parameters.remove(TableProperties.CURRENT_SNAPSHOT_SUMMARY); + LOG.warn("Failed to convert current snapshot({}) summary to a json string", currentSnapshot.snapshotId(), e); + } + } + private StorageDescriptor storageDescriptor(TableMetadata metadata, boolean hiveEngineEnabled) { final StorageDescriptor storageDescriptor = new StorageDescriptor(); diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 0e33663001fb..56881f8272b2 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -19,9 +19,12 @@ package org.apache.iceberg.hive; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.UUID; + +import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hive.metastore.api.Database; import org.apache.iceberg.AssertHelpers; import org.apache.iceberg.CachingCatalog; @@ -30,6 +33,7 @@ import org.apache.iceberg.FileFormat; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.Schema; +import org.apache.iceberg.Snapshot; import org.apache.iceberg.SortOrder; import org.apache.iceberg.Table; import org.apache.iceberg.TableProperties; @@ -57,6 +61,10 @@ import static org.apache.iceberg.NullOrder.NULLS_FIRST; import static org.apache.iceberg.SortDirection.ASC; import static org.apache.iceberg.types.Types.NestedField.required; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.when; public class TestHiveCatalog extends HiveMetastoreTest { private static ImmutableMap meta = ImmutableMap.of( @@ -519,6 +527,52 @@ public void testSnapshotStatsTableProperties() throws Exception { Assert.assertEquals(String.valueOf(snapshotId), parameters.get(TableProperties.CURRENT_SNAPSHOT_ID)); Assert.assertEquals(String.valueOf(icebergTable.currentSnapshot().timestampMillis()), parameters.get(TableProperties.CURRENT_SNAPSHOT_TIMESTAMP)); + + } finally { + catalog.dropTable(tableIdentifier); + } + } + + @Test + public void testSetSnapshotSummary() throws Exception { + Schema schema = new Schema( + required(1, "id", Types.IntegerType.get(), "unique ID"), + required(2, "data", Types.StringType.get()) + ); + TableIdentifier tableIdentifier = TableIdentifier.of(DB_NAME, "tbl"); + String location = temp.newFolder("tbl").toString(); + try { + Table table = catalog.buildTable(tableIdentifier, schema) + .withLocation(location) + .create(); + + String tableName = tableIdentifier.name(); + CachedClientPool spyCachedClientPool = spy(new CachedClientPool(hiveConf, Collections.emptyMap())); + Configuration conf = new Configuration(); + conf.set("iceberg.hive.legacy.table.parameter.size", "true"); + HiveTableOperations spyOps = spy(new HiveTableOperations(conf, spyCachedClientPool, table.io(), + catalog.name(), tableIdentifier.namespace().level(0), tableName)); + Snapshot snapshot = mock(Snapshot.class); + Map summary = Maps.newHashMap(); + // create a snapshot summary, whose json string is less than the max size + for (int i = 0; i < 100; i++) { + summary.put(String.valueOf(i), "value"); + } + Assert.assertTrue(JsonUtil.mapper().writeValueAsString(summary).length() < 4000); + + when(snapshot.summary()).thenReturn(summary); + Map parameter = Maps.newHashMap(); + spyOps.setSnapshotSummary(parameter, snapshot); + + Assert.assertEquals("The snapshot summary must be in parameters", 1, parameter.size()); + + // increase the snapshot summary size so that it exceeds the limit + for (int i = 0; i < 1000; i++) { + summary.put(String.valueOf(i), "value"); + } + Assert.assertTrue(JsonUtil.mapper().writeValueAsString(summary).length() > 4000); + spyOps.setSnapshotSummary(parameter, snapshot); + Assert.assertEquals("The snapshot summary must not be in parameters due to the size limit", 0, parameter.size()); } finally { catalog.dropTable(tableIdentifier); } From 3a11d9b2b7dfa16baacf3d78600f06855beda76f Mon Sep 17 00:00:00 2001 From: yufeigu Date: Wed, 6 Apr 2022 12:38:35 -0700 Subject: [PATCH 06/13] Resolve comments --- .../test/java/org/apache/iceberg/hive/TestHiveCatalog.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 56881f8272b2..969a2d80eb5b 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -23,7 +23,6 @@ import java.util.List; import java.util.Map; import java.util.UUID; - import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hive.metastore.api.Database; import org.apache.iceberg.AssertHelpers; @@ -61,7 +60,6 @@ import static org.apache.iceberg.NullOrder.NULLS_FIRST; import static org.apache.iceberg.SortDirection.ASC; import static org.apache.iceberg.types.Types.NestedField.required; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; @@ -554,7 +552,7 @@ public void testSetSnapshotSummary() throws Exception { catalog.name(), tableIdentifier.namespace().level(0), tableName)); Snapshot snapshot = mock(Snapshot.class); Map summary = Maps.newHashMap(); - // create a snapshot summary, whose json string is less than the max size + // create a snapshot summary whose json string size is less than the limit for (int i = 0; i < 100; i++) { summary.put(String.valueOf(i), "value"); } @@ -566,7 +564,7 @@ public void testSetSnapshotSummary() throws Exception { Assert.assertEquals("The snapshot summary must be in parameters", 1, parameter.size()); - // increase the snapshot summary size so that it exceeds the limit + // create a snapshot summary whose json string size exceeds the limit for (int i = 0; i < 1000; i++) { summary.put(String.valueOf(i), "value"); } From 72bd3beef90c168e923a53dae1270340bcda29fc Mon Sep 17 00:00:00 2001 From: yufeigu Date: Wed, 6 Apr 2022 12:57:30 -0700 Subject: [PATCH 07/13] Simplify the test case testSetSnapshotSummary --- .../apache/iceberg/hive/TestHiveCatalog.java | 64 +++++++------------ 1 file changed, 23 insertions(+), 41 deletions(-) diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 969a2d80eb5b..6df40404edbb 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -19,7 +19,6 @@ package org.apache.iceberg.hive; -import java.util.Collections; import java.util.List; import java.util.Map; import java.util.UUID; @@ -533,46 +532,29 @@ public void testSnapshotStatsTableProperties() throws Exception { @Test public void testSetSnapshotSummary() throws Exception { - Schema schema = new Schema( - required(1, "id", Types.IntegerType.get(), "unique ID"), - required(2, "data", Types.StringType.get()) - ); - TableIdentifier tableIdentifier = TableIdentifier.of(DB_NAME, "tbl"); - String location = temp.newFolder("tbl").toString(); - try { - Table table = catalog.buildTable(tableIdentifier, schema) - .withLocation(location) - .create(); - - String tableName = tableIdentifier.name(); - CachedClientPool spyCachedClientPool = spy(new CachedClientPool(hiveConf, Collections.emptyMap())); - Configuration conf = new Configuration(); - conf.set("iceberg.hive.legacy.table.parameter.size", "true"); - HiveTableOperations spyOps = spy(new HiveTableOperations(conf, spyCachedClientPool, table.io(), - catalog.name(), tableIdentifier.namespace().level(0), tableName)); - Snapshot snapshot = mock(Snapshot.class); - Map summary = Maps.newHashMap(); - // create a snapshot summary whose json string size is less than the limit - for (int i = 0; i < 100; i++) { - summary.put(String.valueOf(i), "value"); - } - Assert.assertTrue(JsonUtil.mapper().writeValueAsString(summary).length() < 4000); - - when(snapshot.summary()).thenReturn(summary); - Map parameter = Maps.newHashMap(); - spyOps.setSnapshotSummary(parameter, snapshot); - - Assert.assertEquals("The snapshot summary must be in parameters", 1, parameter.size()); - - // create a snapshot summary whose json string size exceeds the limit - for (int i = 0; i < 1000; i++) { - summary.put(String.valueOf(i), "value"); - } - Assert.assertTrue(JsonUtil.mapper().writeValueAsString(summary).length() > 4000); - spyOps.setSnapshotSummary(parameter, snapshot); - Assert.assertEquals("The snapshot summary must not be in parameters due to the size limit", 0, parameter.size()); - } finally { - catalog.dropTable(tableIdentifier); + Configuration conf = new Configuration(); + conf.set("iceberg.hive.legacy.table.parameter.size", "true"); + HiveTableOperations spyOps = spy(new HiveTableOperations(conf, null, null, catalog.name(), DB_NAME, "tbl")); + Snapshot snapshot = mock(Snapshot.class); + Map summary = Maps.newHashMap(); + when(snapshot.summary()).thenReturn(summary); + + // create a snapshot summary whose json string size is less than the limit + for (int i = 0; i < 100; i++) { + summary.put(String.valueOf(i), "value"); + } + Assert.assertTrue(JsonUtil.mapper().writeValueAsString(summary).length() < 4000); + Map parameter = Maps.newHashMap(); + spyOps.setSnapshotSummary(parameter, snapshot); + Assert.assertEquals("The snapshot summary must be in parameters", 1, parameter.size()); + + // create a snapshot summary whose json string size exceeds the limit + for (int i = 0; i < 1000; i++) { + summary.put(String.valueOf(i), "value"); } + long summarySize = JsonUtil.mapper().writeValueAsString(summary).length(); + Assert.assertTrue(summarySize > 4000 && summarySize < 32672); + spyOps.setSnapshotSummary(parameter, snapshot); + Assert.assertEquals("The snapshot summary must not be in parameters due to the size limit", 0, parameter.size()); } } From 4daa05aec5031fb69ede2319bb363239dc6a28ad Mon Sep 17 00:00:00 2001 From: yufeigu Date: Wed, 6 Apr 2022 17:05:15 -0700 Subject: [PATCH 08/13] Allow user to config the max size --- .../java/org/apache/iceberg/hive/HiveTableOperations.java | 6 ++---- .../test/java/org/apache/iceberg/hive/TestHiveCatalog.java | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java index 283fa23ad0c9..266eb3ca45ec 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java @@ -92,9 +92,8 @@ public class HiveTableOperations extends BaseMetastoreTableOperations { private static final String HIVE_LOCK_CHECK_MAX_WAIT_MS = "iceberg.hive.lock-check-max-wait-ms"; private static final String HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES = "iceberg.hive.metadata-refresh-max-retries"; private static final String HIVE_TABLE_LEVEL_LOCK_EVICT_MS = "iceberg.hive.table-level-lock-evict-ms"; - private static final String HIVE_LEGACY_TABLE_PARAMETER_SIZE = "iceberg.hive.legacy.table.parameter.size"; + private static final String HIVE_TABLE_PARAMETER_SIZE_MAX = "iceberg.hive.table.parameter.size.max"; private static final long HIVE_TABLE_PARAMETER_SIZE_MAX_DEFAULT = 32672; - private static final long HIVE_TABLE_PARAMETER_SIZE_MAX_LEGACY = 4000; private static final long HIVE_ACQUIRE_LOCK_TIMEOUT_MS_DEFAULT = 3 * 60 * 1000; // 3 minutes private static final long HIVE_LOCK_CHECK_MIN_WAIT_MS_DEFAULT = 50; // 50 milliseconds private static final long HIVE_LOCK_CHECK_MAX_WAIT_MS_DEFAULT = 5 * 1000; // 5 seconds @@ -176,8 +175,7 @@ protected HiveTableOperations(Configuration conf, ClientPool metaClients, FileIO conf.getInt(HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES, HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES_DEFAULT); long tableLevelLockCacheEvictionTimeout = conf.getLong(HIVE_TABLE_LEVEL_LOCK_EVICT_MS, HIVE_TABLE_LEVEL_LOCK_EVICT_MS_DEFAULT); - this.maxHiveTableParameterSize = conf.getBoolean(HIVE_LEGACY_TABLE_PARAMETER_SIZE, false) ? - HIVE_TABLE_PARAMETER_SIZE_MAX_LEGACY : HIVE_TABLE_PARAMETER_SIZE_MAX_DEFAULT; + this.maxHiveTableParameterSize = conf.getLong(HIVE_TABLE_PARAMETER_SIZE_MAX, HIVE_TABLE_PARAMETER_SIZE_MAX_DEFAULT); initTableLevelLockCache(tableLevelLockCacheEvictionTimeout); } diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 6df40404edbb..45696c268382 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -533,7 +533,7 @@ public void testSnapshotStatsTableProperties() throws Exception { @Test public void testSetSnapshotSummary() throws Exception { Configuration conf = new Configuration(); - conf.set("iceberg.hive.legacy.table.parameter.size", "true"); + conf.set("iceberg.hive.table.parameter.size.max", "4000"); HiveTableOperations spyOps = spy(new HiveTableOperations(conf, null, null, catalog.name(), DB_NAME, "tbl")); Snapshot snapshot = mock(Snapshot.class); Map summary = Maps.newHashMap(); From 8f5edaa2ea46dc35656615be0991695552f29cc7 Mon Sep 17 00:00:00 2001 From: yufeigu Date: Thu, 7 Apr 2022 11:39:50 -0700 Subject: [PATCH 09/13] Resolve comments --- .../java/org/apache/iceberg/hive/HiveTableOperations.java | 4 +++- .../test/java/org/apache/iceberg/hive/TestHiveCatalog.java | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java index 266eb3ca45ec..fcebed37f7af 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java @@ -92,6 +92,8 @@ public class HiveTableOperations extends BaseMetastoreTableOperations { private static final String HIVE_LOCK_CHECK_MAX_WAIT_MS = "iceberg.hive.lock-check-max-wait-ms"; private static final String HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES = "iceberg.hive.metadata-refresh-max-retries"; private static final String HIVE_TABLE_LEVEL_LOCK_EVICT_MS = "iceberg.hive.table-level-lock-evict-ms"; + + // determined by the HMS backend database, https://issues.apache.org/jira/browse/HIVE-12274 private static final String HIVE_TABLE_PARAMETER_SIZE_MAX = "iceberg.hive.table.parameter.size.max"; private static final long HIVE_TABLE_PARAMETER_SIZE_MAX_DEFAULT = 32672; private static final long HIVE_ACQUIRE_LOCK_TIMEOUT_MS_DEFAULT = 3 * 60 * 1000; // 3 minutes @@ -173,9 +175,9 @@ protected HiveTableOperations(Configuration conf, ClientPool metaClients, FileIO conf.getLong(HIVE_LOCK_CHECK_MAX_WAIT_MS, HIVE_LOCK_CHECK_MAX_WAIT_MS_DEFAULT); this.metadataRefreshMaxRetries = conf.getInt(HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES, HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES_DEFAULT); + this.maxHiveTableParameterSize = conf.getLong(HIVE_TABLE_PARAMETER_SIZE_MAX, HIVE_TABLE_PARAMETER_SIZE_MAX_DEFAULT); long tableLevelLockCacheEvictionTimeout = conf.getLong(HIVE_TABLE_LEVEL_LOCK_EVICT_MS, HIVE_TABLE_LEVEL_LOCK_EVICT_MS_DEFAULT); - this.maxHiveTableParameterSize = conf.getLong(HIVE_TABLE_PARAMETER_SIZE_MAX, HIVE_TABLE_PARAMETER_SIZE_MAX_DEFAULT); initTableLevelLockCache(tableLevelLockCacheEvictionTimeout); } diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 45696c268382..2500fa65c1d2 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -553,6 +553,7 @@ public void testSetSnapshotSummary() throws Exception { summary.put(String.valueOf(i), "value"); } long summarySize = JsonUtil.mapper().writeValueAsString(summary).length(); + // the limit has been updated to 4000 instead of the default value(32672) Assert.assertTrue(summarySize > 4000 && summarySize < 32672); spyOps.setSnapshotSummary(parameter, snapshot); Assert.assertEquals("The snapshot summary must not be in parameters due to the size limit", 0, parameter.size()); From cd726fe650b692a8d73058a4345b4c9221c4750e Mon Sep 17 00:00:00 2001 From: yufeigu Date: Thu, 7 Apr 2022 14:27:55 -0700 Subject: [PATCH 10/13] Resolve comments --- .../java/org/apache/iceberg/hive/HiveTableOperations.java | 5 +++-- .../test/java/org/apache/iceberg/hive/TestHiveCatalog.java | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java index fcebed37f7af..a819fd0f77fd 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java @@ -93,8 +93,9 @@ public class HiveTableOperations extends BaseMetastoreTableOperations { private static final String HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES = "iceberg.hive.metadata-refresh-max-retries"; private static final String HIVE_TABLE_LEVEL_LOCK_EVICT_MS = "iceberg.hive.table-level-lock-evict-ms"; - // determined by the HMS backend database, https://issues.apache.org/jira/browse/HIVE-12274 - private static final String HIVE_TABLE_PARAMETER_SIZE_MAX = "iceberg.hive.table.parameter.size.max"; + // the max size is based on HMS backend database. For Hive versions below 2.3, the max table parameter size is 4000 + // characters, see https://issues.apache.org/jira/browse/HIVE-12274 + private static final String HIVE_TABLE_PARAMETER_SIZE_MAX = "iceberg.hive.max.table.parameter.size"; private static final long HIVE_TABLE_PARAMETER_SIZE_MAX_DEFAULT = 32672; private static final long HIVE_ACQUIRE_LOCK_TIMEOUT_MS_DEFAULT = 3 * 60 * 1000; // 3 minutes private static final long HIVE_LOCK_CHECK_MIN_WAIT_MS_DEFAULT = 50; // 50 milliseconds diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 2500fa65c1d2..ba59fc1d67fb 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -533,7 +533,7 @@ public void testSnapshotStatsTableProperties() throws Exception { @Test public void testSetSnapshotSummary() throws Exception { Configuration conf = new Configuration(); - conf.set("iceberg.hive.table.parameter.size.max", "4000"); + conf.set("iceberg.hive.max.table.parameter.size", "4000"); HiveTableOperations spyOps = spy(new HiveTableOperations(conf, null, null, catalog.name(), DB_NAME, "tbl")); Snapshot snapshot = mock(Snapshot.class); Map summary = Maps.newHashMap(); From 1e0d23f4f32f941d436d59d6af07711e376436e1 Mon Sep 17 00:00:00 2001 From: yufeigu Date: Thu, 7 Apr 2022 15:24:06 -0700 Subject: [PATCH 11/13] Resolve comments --- .../java/org/apache/iceberg/hive/HiveTableOperations.java | 6 +++--- .../test/java/org/apache/iceberg/hive/TestHiveCatalog.java | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java index a819fd0f77fd..a503d1ab7a5e 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java @@ -95,8 +95,8 @@ public class HiveTableOperations extends BaseMetastoreTableOperations { // the max size is based on HMS backend database. For Hive versions below 2.3, the max table parameter size is 4000 // characters, see https://issues.apache.org/jira/browse/HIVE-12274 - private static final String HIVE_TABLE_PARAMETER_SIZE_MAX = "iceberg.hive.max.table.parameter.size"; - private static final long HIVE_TABLE_PARAMETER_SIZE_MAX_DEFAULT = 32672; + private static final String HIVE_TABLE_PARAMETER_MAX_SIZE = "iceberg.hive.table-parameter-max-size"; + private static final long HIVE_TABLE_PARAMETER_MAX_SIZE_DEFAULT = 32672; private static final long HIVE_ACQUIRE_LOCK_TIMEOUT_MS_DEFAULT = 3 * 60 * 1000; // 3 minutes private static final long HIVE_LOCK_CHECK_MIN_WAIT_MS_DEFAULT = 50; // 50 milliseconds private static final long HIVE_LOCK_CHECK_MAX_WAIT_MS_DEFAULT = 5 * 1000; // 5 seconds @@ -176,7 +176,7 @@ protected HiveTableOperations(Configuration conf, ClientPool metaClients, FileIO conf.getLong(HIVE_LOCK_CHECK_MAX_WAIT_MS, HIVE_LOCK_CHECK_MAX_WAIT_MS_DEFAULT); this.metadataRefreshMaxRetries = conf.getInt(HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES, HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES_DEFAULT); - this.maxHiveTableParameterSize = conf.getLong(HIVE_TABLE_PARAMETER_SIZE_MAX, HIVE_TABLE_PARAMETER_SIZE_MAX_DEFAULT); + this.maxHiveTableParameterSize = conf.getLong(HIVE_TABLE_PARAMETER_MAX_SIZE, HIVE_TABLE_PARAMETER_MAX_SIZE_DEFAULT); long tableLevelLockCacheEvictionTimeout = conf.getLong(HIVE_TABLE_LEVEL_LOCK_EVICT_MS, HIVE_TABLE_LEVEL_LOCK_EVICT_MS_DEFAULT); initTableLevelLockCache(tableLevelLockCacheEvictionTimeout); diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index ba59fc1d67fb..6733599e82fe 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -533,7 +533,7 @@ public void testSnapshotStatsTableProperties() throws Exception { @Test public void testSetSnapshotSummary() throws Exception { Configuration conf = new Configuration(); - conf.set("iceberg.hive.max.table.parameter.size", "4000"); + conf.set("iceberg.hive.table-parameter-max-size", "4000"); HiveTableOperations spyOps = spy(new HiveTableOperations(conf, null, null, catalog.name(), DB_NAME, "tbl")); Snapshot snapshot = mock(Snapshot.class); Map summary = Maps.newHashMap(); From a3bea2f0c0bdc94d3376126969b42ed0550f787b Mon Sep 17 00:00:00 2001 From: yufeigu Date: Thu, 7 Apr 2022 15:29:25 -0700 Subject: [PATCH 12/13] Resolve comments --- .../org/apache/iceberg/hive/HiveTableOperations.java | 12 ++++++------ .../org/apache/iceberg/hive/TestHiveCatalog.java | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java index a503d1ab7a5e..b7270524e5a0 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java @@ -95,8 +95,8 @@ public class HiveTableOperations extends BaseMetastoreTableOperations { // the max size is based on HMS backend database. For Hive versions below 2.3, the max table parameter size is 4000 // characters, see https://issues.apache.org/jira/browse/HIVE-12274 - private static final String HIVE_TABLE_PARAMETER_MAX_SIZE = "iceberg.hive.table-parameter-max-size"; - private static final long HIVE_TABLE_PARAMETER_MAX_SIZE_DEFAULT = 32672; + private static final String HIVE_TABLE_PROPERTY_MAX_SIZE = "iceberg.hive.table-property-max-size"; + private static final long HIVE_TABLE_PROPERTY_MAX_SIZE_DEFAULT = 32672; private static final long HIVE_ACQUIRE_LOCK_TIMEOUT_MS_DEFAULT = 3 * 60 * 1000; // 3 minutes private static final long HIVE_LOCK_CHECK_MIN_WAIT_MS_DEFAULT = 50; // 50 milliseconds private static final long HIVE_LOCK_CHECK_MAX_WAIT_MS_DEFAULT = 5 * 1000; // 5 seconds @@ -155,7 +155,7 @@ private static class WaitingForLockException extends RuntimeException { private final long lockAcquireTimeout; private final long lockCheckMinWaitTime; private final long lockCheckMaxWaitTime; - private final long maxHiveTableParameterSize; + private final long maxHiveTablePropertySize; private final int metadataRefreshMaxRetries; private final FileIO fileIO; private final ClientPool metaClients; @@ -176,7 +176,7 @@ protected HiveTableOperations(Configuration conf, ClientPool metaClients, FileIO conf.getLong(HIVE_LOCK_CHECK_MAX_WAIT_MS, HIVE_LOCK_CHECK_MAX_WAIT_MS_DEFAULT); this.metadataRefreshMaxRetries = conf.getInt(HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES, HIVE_ICEBERG_METADATA_REFRESH_MAX_RETRIES_DEFAULT); - this.maxHiveTableParameterSize = conf.getLong(HIVE_TABLE_PARAMETER_MAX_SIZE, HIVE_TABLE_PARAMETER_MAX_SIZE_DEFAULT); + this.maxHiveTablePropertySize = conf.getLong(HIVE_TABLE_PROPERTY_MAX_SIZE, HIVE_TABLE_PROPERTY_MAX_SIZE_DEFAULT); long tableLevelLockCacheEvictionTimeout = conf.getLong(HIVE_TABLE_LEVEL_LOCK_EVICT_MS, HIVE_TABLE_LEVEL_LOCK_EVICT_MS_DEFAULT); initTableLevelLockCache(tableLevelLockCacheEvictionTimeout); @@ -435,12 +435,12 @@ private void setSnapshotStats(TableMetadata metadata, Map parame void setSnapshotSummary(Map parameters, Snapshot currentSnapshot) { try { String summary = JsonUtil.mapper().writeValueAsString(currentSnapshot.summary()); - if (summary.length() <= maxHiveTableParameterSize) { + if (summary.length() <= maxHiveTablePropertySize) { parameters.put(TableProperties.CURRENT_SNAPSHOT_SUMMARY, summary); } else { parameters.remove(TableProperties.CURRENT_SNAPSHOT_SUMMARY); LOG.warn("Not exposing the current snapshot({}) summary in HMS since it exceeds {} characters", - currentSnapshot.snapshotId(), maxHiveTableParameterSize); + currentSnapshot.snapshotId(), maxHiveTablePropertySize); } } catch (JsonProcessingException e) { parameters.remove(TableProperties.CURRENT_SNAPSHOT_SUMMARY); diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 6733599e82fe..5599163d9e90 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -533,7 +533,7 @@ public void testSnapshotStatsTableProperties() throws Exception { @Test public void testSetSnapshotSummary() throws Exception { Configuration conf = new Configuration(); - conf.set("iceberg.hive.table-parameter-max-size", "4000"); + conf.set("iceberg.hive.table-property-max-size", "4000"); HiveTableOperations spyOps = spy(new HiveTableOperations(conf, null, null, catalog.name(), DB_NAME, "tbl")); Snapshot snapshot = mock(Snapshot.class); Map summary = Maps.newHashMap(); From 617d6654df89e7da742fb0c3495f149321944072 Mon Sep 17 00:00:00 2001 From: yufeigu Date: Thu, 7 Apr 2022 17:08:30 -0700 Subject: [PATCH 13/13] Resolve comments --- .../org/apache/iceberg/hive/HiveTableOperations.java | 10 ++++------ .../java/org/apache/iceberg/hive/TestHiveCatalog.java | 11 ++++++----- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java index b7270524e5a0..88374ea0df65 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java @@ -417,15 +417,15 @@ private void setHmsTableParameters(String newMetadataLocation, Table tbl, TableM } private void setSnapshotStats(TableMetadata metadata, Map parameters) { + parameters.remove(TableProperties.CURRENT_SNAPSHOT_ID); + parameters.remove(TableProperties.CURRENT_SNAPSHOT_TIMESTAMP); + parameters.remove(TableProperties.CURRENT_SNAPSHOT_SUMMARY); + Snapshot currentSnapshot = metadata.currentSnapshot(); if (currentSnapshot != null) { parameters.put(TableProperties.CURRENT_SNAPSHOT_ID, String.valueOf(currentSnapshot.snapshotId())); parameters.put(TableProperties.CURRENT_SNAPSHOT_TIMESTAMP, String.valueOf(currentSnapshot.timestampMillis())); setSnapshotSummary(parameters, currentSnapshot); - } else { - parameters.remove(TableProperties.CURRENT_SNAPSHOT_ID); - parameters.remove(TableProperties.CURRENT_SNAPSHOT_TIMESTAMP); - parameters.remove(TableProperties.CURRENT_SNAPSHOT_SUMMARY); } parameters.put(TableProperties.SNAPSHOT_COUNT, String.valueOf(metadata.snapshots().size())); @@ -438,12 +438,10 @@ void setSnapshotSummary(Map parameters, Snapshot currentSnapshot if (summary.length() <= maxHiveTablePropertySize) { parameters.put(TableProperties.CURRENT_SNAPSHOT_SUMMARY, summary); } else { - parameters.remove(TableProperties.CURRENT_SNAPSHOT_SUMMARY); LOG.warn("Not exposing the current snapshot({}) summary in HMS since it exceeds {} characters", currentSnapshot.snapshotId(), maxHiveTablePropertySize); } } catch (JsonProcessingException e) { - parameters.remove(TableProperties.CURRENT_SNAPSHOT_SUMMARY); LOG.warn("Failed to convert current snapshot({}) summary to a json string", currentSnapshot.snapshotId(), e); } } diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 5599163d9e90..40011625cdc5 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -544,9 +544,9 @@ public void testSetSnapshotSummary() throws Exception { summary.put(String.valueOf(i), "value"); } Assert.assertTrue(JsonUtil.mapper().writeValueAsString(summary).length() < 4000); - Map parameter = Maps.newHashMap(); - spyOps.setSnapshotSummary(parameter, snapshot); - Assert.assertEquals("The snapshot summary must be in parameters", 1, parameter.size()); + Map parameters = Maps.newHashMap(); + spyOps.setSnapshotSummary(parameters, snapshot); + Assert.assertEquals("The snapshot summary must be in parameters", 1, parameters.size()); // create a snapshot summary whose json string size exceeds the limit for (int i = 0; i < 1000; i++) { @@ -555,7 +555,8 @@ public void testSetSnapshotSummary() throws Exception { long summarySize = JsonUtil.mapper().writeValueAsString(summary).length(); // the limit has been updated to 4000 instead of the default value(32672) Assert.assertTrue(summarySize > 4000 && summarySize < 32672); - spyOps.setSnapshotSummary(parameter, snapshot); - Assert.assertEquals("The snapshot summary must not be in parameters due to the size limit", 0, parameter.size()); + parameters.remove(TableProperties.CURRENT_SNAPSHOT_SUMMARY); + spyOps.setSnapshotSummary(parameters, snapshot); + Assert.assertEquals("The snapshot summary must not be in parameters due to the size limit", 0, parameters.size()); } }