From fbbe2ccb00cc097312cfe0454918e04738367ea5 Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Fri, 19 Jul 2019 12:04:35 -0700 Subject: [PATCH 1/5] Do not fail writes when an invalid metrics mode is in table config. --- .../java/org/apache/iceberg/MetricsConfig.java | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/MetricsConfig.java b/core/src/main/java/org/apache/iceberg/MetricsConfig.java index 864ec060495e..f7e19a85202b 100644 --- a/core/src/main/java/org/apache/iceberg/MetricsConfig.java +++ b/core/src/main/java/org/apache/iceberg/MetricsConfig.java @@ -22,12 +22,15 @@ import com.google.common.collect.Maps; import java.util.Map; import org.apache.iceberg.MetricsModes.MetricsMode; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import static org.apache.iceberg.TableProperties.DEFAULT_WRITE_METRICS_MODE; import static org.apache.iceberg.TableProperties.DEFAULT_WRITE_METRICS_MODE_DEFAULT; public class MetricsConfig { + private static final Logger LOG = LoggerFactory.getLogger(MetricsConfig.class); private static final String COLUMN_CONF_PREFIX = "write.metadata.metrics.column."; private Map columnModes = Maps.newHashMap(); @@ -43,15 +46,24 @@ public static MetricsConfig getDefault() { public static MetricsConfig fromProperties(Map props) { MetricsConfig spec = new MetricsConfig(); + String defaultModeAsString = props.getOrDefault(DEFAULT_WRITE_METRICS_MODE, DEFAULT_WRITE_METRICS_MODE_DEFAULT); + spec.defaultMode = MetricsModes.fromString(defaultModeAsString); + props.keySet().stream() .filter(key -> key.startsWith(COLUMN_CONF_PREFIX)) .forEach(key -> { - MetricsMode mode = MetricsModes.fromString(props.get(key)); String columnAlias = key.replaceFirst(COLUMN_CONF_PREFIX, ""); + MetricsMode mode; + try { + mode = MetricsModes.fromString(props.get(key)); + } catch (IllegalArgumentException e) { + // Mode was invalid, log the error and use the default + LOG.warn("Ignoring invalid metrics mode for column %s: %s", columnAlias, key); + mode = spec.defaultMode; + } spec.columnModes.put(columnAlias, mode); }); - String defaultModeAsString = props.getOrDefault(DEFAULT_WRITE_METRICS_MODE, DEFAULT_WRITE_METRICS_MODE_DEFAULT); - spec.defaultMode = MetricsModes.fromString(defaultModeAsString); + return spec; } From b3d9156953b3ca40664e9802c32594fa88136f5a Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Thu, 1 Aug 2019 10:09:10 -0700 Subject: [PATCH 2/5] Fix MetricsConfig checkstyle. --- core/src/main/java/org/apache/iceberg/MetricsConfig.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/iceberg/MetricsConfig.java b/core/src/main/java/org/apache/iceberg/MetricsConfig.java index f7e19a85202b..a4a84f9fd075 100644 --- a/core/src/main/java/org/apache/iceberg/MetricsConfig.java +++ b/core/src/main/java/org/apache/iceberg/MetricsConfig.java @@ -44,6 +44,7 @@ public static MetricsConfig getDefault() { return spec; } + @SuppressWarnings("checkstyle:CatchBlockLogException") public static MetricsConfig fromProperties(Map props) { MetricsConfig spec = new MetricsConfig(); String defaultModeAsString = props.getOrDefault(DEFAULT_WRITE_METRICS_MODE, DEFAULT_WRITE_METRICS_MODE_DEFAULT); @@ -56,7 +57,7 @@ public static MetricsConfig fromProperties(Map props) { MetricsMode mode; try { mode = MetricsModes.fromString(props.get(key)); - } catch (IllegalArgumentException e) { + } catch (IllegalArgumentException ignored) { // Mode was invalid, log the error and use the default LOG.warn("Ignoring invalid metrics mode for column %s: %s", columnAlias, key); mode = spec.defaultMode; From 50af719f45e2c7890d3c1577667dab0fc5dc52c7 Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Thu, 1 Aug 2019 10:12:01 -0700 Subject: [PATCH 3/5] Catch invalid default metrics mode. --- core/src/main/java/org/apache/iceberg/MetricsConfig.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/iceberg/MetricsConfig.java b/core/src/main/java/org/apache/iceberg/MetricsConfig.java index a4a84f9fd075..ba0d8ecc5917 100644 --- a/core/src/main/java/org/apache/iceberg/MetricsConfig.java +++ b/core/src/main/java/org/apache/iceberg/MetricsConfig.java @@ -48,7 +48,13 @@ public static MetricsConfig getDefault() { public static MetricsConfig fromProperties(Map props) { MetricsConfig spec = new MetricsConfig(); String defaultModeAsString = props.getOrDefault(DEFAULT_WRITE_METRICS_MODE, DEFAULT_WRITE_METRICS_MODE_DEFAULT); - spec.defaultMode = MetricsModes.fromString(defaultModeAsString); + try { + spec.defaultMode = MetricsModes.fromString(defaultModeAsString); + } catch (IllegalArgumentException ignored) { + // Mode was invalid, log the error and use the default + LOG.warn("Ignoring invalid default metrics mode: %s", defaultModeAsString); + spec.defaultMode = MetricsModes.fromString(DEFAULT_WRITE_METRICS_MODE_DEFAULT); + } props.keySet().stream() .filter(key -> key.startsWith(COLUMN_CONF_PREFIX)) From 88bd776bef458743850a01865f47675a69790d38 Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Thu, 1 Aug 2019 10:21:26 -0700 Subject: [PATCH 4/5] Add tests and fix logging. --- .../org/apache/iceberg/MetricsConfig.java | 9 ++++--- .../org/apache/iceberg/TableProperties.java | 1 + .../org/apache/iceberg/TestMetricsModes.java | 24 +++++++++++++++++++ 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/MetricsConfig.java b/core/src/main/java/org/apache/iceberg/MetricsConfig.java index ba0d8ecc5917..518fca7737ee 100644 --- a/core/src/main/java/org/apache/iceberg/MetricsConfig.java +++ b/core/src/main/java/org/apache/iceberg/MetricsConfig.java @@ -31,7 +31,6 @@ public class MetricsConfig { private static final Logger LOG = LoggerFactory.getLogger(MetricsConfig.class); - private static final String COLUMN_CONF_PREFIX = "write.metadata.metrics.column."; private Map columnModes = Maps.newHashMap(); private MetricsMode defaultMode; @@ -52,20 +51,20 @@ public static MetricsConfig fromProperties(Map props) { spec.defaultMode = MetricsModes.fromString(defaultModeAsString); } catch (IllegalArgumentException ignored) { // Mode was invalid, log the error and use the default - LOG.warn("Ignoring invalid default metrics mode: %s", defaultModeAsString); + LOG.warn("Ignoring invalid default metrics mode: {}", defaultModeAsString); spec.defaultMode = MetricsModes.fromString(DEFAULT_WRITE_METRICS_MODE_DEFAULT); } props.keySet().stream() - .filter(key -> key.startsWith(COLUMN_CONF_PREFIX)) + .filter(key -> key.startsWith(TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX)) .forEach(key -> { - String columnAlias = key.replaceFirst(COLUMN_CONF_PREFIX, ""); + String columnAlias = key.replaceFirst(TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX, ""); MetricsMode mode; try { mode = MetricsModes.fromString(props.get(key)); } catch (IllegalArgumentException ignored) { // Mode was invalid, log the error and use the default - LOG.warn("Ignoring invalid metrics mode for column %s: %s", columnAlias, key); + LOG.warn("Ignoring invalid metrics mode for column {}: {}", columnAlias, props.get(key)); mode = spec.defaultMode; } spec.columnModes.put(columnAlias, mode); diff --git a/core/src/main/java/org/apache/iceberg/TableProperties.java b/core/src/main/java/org/apache/iceberg/TableProperties.java index ecb0f31d57ca..42a43dc8a1c2 100644 --- a/core/src/main/java/org/apache/iceberg/TableProperties.java +++ b/core/src/main/java/org/apache/iceberg/TableProperties.java @@ -89,6 +89,7 @@ private TableProperties() {} public static final String METADATA_COMPRESSION = "write.metadata.compression-codec"; public static final String METADATA_COMPRESSION_DEFAULT = "none"; + public static final String METRICS_MODE_COLUMN_CONF_PREFIX = "write.metadata.metrics.column."; public static final String DEFAULT_WRITE_METRICS_MODE = "write.metadata.metrics.default"; public static final String DEFAULT_WRITE_METRICS_MODE_DEFAULT = "truncate(16)"; } diff --git a/core/src/test/java/org/apache/iceberg/TestMetricsModes.java b/core/src/test/java/org/apache/iceberg/TestMetricsModes.java index 03b6bb792202..698f5c3a193f 100644 --- a/core/src/test/java/org/apache/iceberg/TestMetricsModes.java +++ b/core/src/test/java/org/apache/iceberg/TestMetricsModes.java @@ -19,6 +19,8 @@ package org.apache.iceberg; +import com.google.common.collect.ImmutableMap; +import java.util.Map; import org.apache.iceberg.MetricsModes.Counts; import org.apache.iceberg.MetricsModes.Full; import org.apache.iceberg.MetricsModes.None; @@ -51,4 +53,26 @@ public void testInvalidTruncationLength() { exceptionRule.expectMessage("length should be positive"); MetricsModes.fromString("truncate(0)"); } + + @Test + public void testInvalidColumnModeValue() { + Map properties = ImmutableMap.of( + TableProperties.DEFAULT_WRITE_METRICS_MODE, "full", + TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX + "col", "troncate(5)"); + + MetricsConfig config = MetricsConfig.fromProperties(properties); + Assert.assertEquals("Invalid mode should be defaulted to table default (full)", + MetricsModes.Full.get(), config.columnMode("col")); + } + + @Test + public void testInvalidDefaultColumnModeValue() { + Map properties = ImmutableMap.of( + TableProperties.DEFAULT_WRITE_METRICS_MODE, "fuull", + TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX + "col", "troncate(5)"); + + MetricsConfig config = MetricsConfig.fromProperties(properties); + Assert.assertEquals("Invalid mode should be defaulted to library default (truncate(16))", + MetricsModes.Truncate.withLength(16), config.columnMode("col")); + } } From f93a528c40a5a17a02b9d0219f7b6daf866a2617 Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Thu, 1 Aug 2019 10:35:03 -0700 Subject: [PATCH 5/5] Fix checkstyle problems. --- core/src/main/java/org/apache/iceberg/MetricsConfig.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/MetricsConfig.java b/core/src/main/java/org/apache/iceberg/MetricsConfig.java index 518fca7737ee..816f23e7ea99 100644 --- a/core/src/main/java/org/apache/iceberg/MetricsConfig.java +++ b/core/src/main/java/org/apache/iceberg/MetricsConfig.java @@ -43,15 +43,14 @@ public static MetricsConfig getDefault() { return spec; } - @SuppressWarnings("checkstyle:CatchBlockLogException") public static MetricsConfig fromProperties(Map props) { MetricsConfig spec = new MetricsConfig(); String defaultModeAsString = props.getOrDefault(DEFAULT_WRITE_METRICS_MODE, DEFAULT_WRITE_METRICS_MODE_DEFAULT); try { spec.defaultMode = MetricsModes.fromString(defaultModeAsString); - } catch (IllegalArgumentException ignored) { + } catch (IllegalArgumentException err) { // Mode was invalid, log the error and use the default - LOG.warn("Ignoring invalid default metrics mode: {}", defaultModeAsString); + LOG.warn("Ignoring invalid default metrics mode: {}", defaultModeAsString, err); spec.defaultMode = MetricsModes.fromString(DEFAULT_WRITE_METRICS_MODE_DEFAULT); } @@ -62,9 +61,9 @@ public static MetricsConfig fromProperties(Map props) { MetricsMode mode; try { mode = MetricsModes.fromString(props.get(key)); - } catch (IllegalArgumentException ignored) { + } catch (IllegalArgumentException err) { // Mode was invalid, log the error and use the default - LOG.warn("Ignoring invalid metrics mode for column {}: {}", columnAlias, props.get(key)); + LOG.warn("Ignoring invalid metrics mode for column {}: {}", columnAlias, props.get(key), err); mode = spec.defaultMode; } spec.columnModes.put(columnAlias, mode);