commit 9f94c1cd2c128e707799e510a7b96b135ae5a7af Author: Alan Gates Date: Thu Aug 31 14:42:38 2017 -0700 HIVE-17425 Change MetastoreConf.ConfVars internal members to be private diff --git metastore/src/java/org/apache/hadoop/hive/metastore/datasource/BoneCPDataSourceProvider.java metastore/src/java/org/apache/hadoop/hive/metastore/datasource/BoneCPDataSourceProvider.java index 34765b0b2f..52543ede9e 100644 --- metastore/src/java/org/apache/hadoop/hive/metastore/datasource/BoneCPDataSourceProvider.java +++ metastore/src/java/org/apache/hadoop/hive/metastore/datasource/BoneCPDataSourceProvider.java @@ -47,9 +47,8 @@ public DataSource create(Configuration hdpConfig) throws SQLException { String driverUrl = DataSourceProvider.getMetastoreJdbcDriverUrl(hdpConfig); String user = DataSourceProvider.getMetastoreJdbcUser(hdpConfig); String passwd = DataSourceProvider.getMetastoreJdbcPasswd(hdpConfig); - int maxPoolSize = hdpConfig.getInt( - MetastoreConf.ConfVars.CONNECTION_POOLING_MAX_CONNECTIONS.varname, - ((Long)MetastoreConf.ConfVars.CONNECTION_POOLING_MAX_CONNECTIONS.defaultVal).intValue()); + int maxPoolSize = MetastoreConf.getIntVar(hdpConfig, + MetastoreConf.ConfVars.CONNECTION_POOLING_MAX_CONNECTIONS); Properties properties = DataSourceProvider.getPrefixedProperties(hdpConfig, BONECP); long connectionTimeout = hdpConfig.getLong(CONNECTION_TIMEOUT_PROPERTY, 30000L); @@ -81,9 +80,8 @@ public boolean mayReturnClosedConnection() { @Override public boolean supports(Configuration configuration) { - String poolingType = - configuration.get( - MetastoreConf.ConfVars.CONNECTION_POOLING_TYPE.varname).toLowerCase(); + String poolingType = MetastoreConf.getVar(configuration, + MetastoreConf.ConfVars.CONNECTION_POOLING_TYPE).toLowerCase(); if (BONECP.equals(poolingType)) { int boneCpPropsNr = DataSourceProvider.getPrefixedProperties(configuration, BONECP).size(); LOG.debug("Found " + boneCpPropsNr + " nr. of bonecp specific configurations"); diff --git metastore/src/java/org/apache/hadoop/hive/metastore/datasource/DataSourceProvider.java metastore/src/java/org/apache/hadoop/hive/metastore/datasource/DataSourceProvider.java index ad1763e121..328716ee39 100644 --- metastore/src/java/org/apache/hadoop/hive/metastore/datasource/DataSourceProvider.java +++ metastore/src/java/org/apache/hadoop/hive/metastore/datasource/DataSourceProvider.java @@ -61,7 +61,7 @@ static Properties getPrefixedProperties(Configuration hdpConfig, String factoryP } static String getMetastoreJdbcUser(Configuration conf) { - return conf.get(MetastoreConf.ConfVars.CONNECTION_USER_NAME.varname); + return MetastoreConf.getVar(conf, MetastoreConf.ConfVars.CONNECTION_USER_NAME); } static String getMetastoreJdbcPasswd(Configuration conf) throws SQLException { @@ -73,7 +73,7 @@ static String getMetastoreJdbcPasswd(Configuration conf) throws SQLException { } static String getMetastoreJdbcDriverUrl(Configuration conf) throws SQLException { - return conf.get(MetastoreConf.ConfVars.CONNECTURLKEY.varname); + return MetastoreConf.getVar(conf, MetastoreConf.ConfVars.CONNECTURLKEY); } } diff --git metastore/src/java/org/apache/hadoop/hive/metastore/datasource/HikariCPDataSourceProvider.java metastore/src/java/org/apache/hadoop/hive/metastore/datasource/HikariCPDataSourceProvider.java index 9b3d6d5d70..5c6b15632c 100644 --- metastore/src/java/org/apache/hadoop/hive/metastore/datasource/HikariCPDataSourceProvider.java +++ metastore/src/java/org/apache/hadoop/hive/metastore/datasource/HikariCPDataSourceProvider.java @@ -46,9 +46,8 @@ public DataSource create(Configuration hdpConfig) throws SQLException { String driverUrl = DataSourceProvider.getMetastoreJdbcDriverUrl(hdpConfig); String user = DataSourceProvider.getMetastoreJdbcUser(hdpConfig); String passwd = DataSourceProvider.getMetastoreJdbcPasswd(hdpConfig); - int maxPoolSize = hdpConfig.getInt( - MetastoreConf.ConfVars.CONNECTION_POOLING_MAX_CONNECTIONS.varname, - ((Long)MetastoreConf.ConfVars.CONNECTION_POOLING_MAX_CONNECTIONS.defaultVal).intValue()); + int maxPoolSize = MetastoreConf.getIntVar(hdpConfig, + MetastoreConf.ConfVars.CONNECTION_POOLING_MAX_CONNECTIONS); Properties properties = replacePrefix( DataSourceProvider.getPrefixedProperties(hdpConfig, HIKARI)); @@ -76,9 +75,8 @@ public boolean mayReturnClosedConnection() { @Override public boolean supports(Configuration configuration) { - String poolingType = - configuration.get( - MetastoreConf.ConfVars.CONNECTION_POOLING_TYPE.varname).toLowerCase(); + String poolingType = MetastoreConf.getVar(configuration, + MetastoreConf.ConfVars.CONNECTION_POOLING_TYPE).toLowerCase(); if (HIKARI.equals(poolingType)) { int hikariPropsNr = DataSourceProvider.getPrefixedProperties(configuration, HIKARI).size(); LOG.debug("Found " + hikariPropsNr + " nr. of hikari specific configurations"); diff --git standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java index 0fb878a267..2cae2a4673 100644 --- standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java +++ standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java @@ -41,6 +41,21 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; +/** + * A set of definitions of config values used by the Metastore. One of the key aims of this + * class is to provide backwards compatibility with existing Hive configuration keys while + * allowing the metastore to have its own, Hive independant keys. For this reason access to the + * underlying Configuration object should always be done via the static methods provided here + * rather than directly via {@link Configuration#get(String)} and + * {@link Configuration#set(String, String)}. All the methods of this class will handle checking + * both the MetastoreConf key and the Hive key. The algorithm is, on reads, to check first the + * MetastoreConf key, then the Hive key, then return the default if neither are set. On write + * the Metastore key only is set. + * + * This class does not extend Configuration. Rather it provides static methods for operating on + * a Configuration object. This allows it to work on HiveConf objects, which otherwise would not + * be the case. + */ public class MetastoreConf { private static final Logger LOG = LoggerFactory.getLogger(MetastoreConf.class); @@ -752,11 +767,11 @@ public static ConfVars getMetaConf(String name) { BOOLEAN_TEST_ENTRY("test.bool", "hive.test.bool", true, "comment"), CLASS_TEST_ENTRY("test.class", "hive.test.class", "", "comment"); - public final String varname; - public final String hiveName; - public final Object defaultVal; - public final Validator validator; - public final boolean caseSensitive; + private final String varname; + private final String hiveName; + private final Object defaultVal; + private final Validator validator; + private final boolean caseSensitive; ConfVars(String varname, String hiveName, String defaultVal, String comment) { this.varname = varname; @@ -844,6 +859,30 @@ public boolean isCaseSensitive() { return caseSensitive; } + /** + * If you are calling this, you're probably doing it wrong. You shouldn't need to use the + * underlying variable name. Use one of the getVar methods instead. Only use this if you + * are 100% sure you know you're doing. The reason for this is that MetastoreConf goes to a + * lot of trouble to make sure it checks both Hive and Metastore values for config keys. If + * you call conf.get(varname) you are undermining that. + * @return variable name + */ + public String getVarname() { + return varname; + } + + /** + * If you are calling this, you're probably doing it wrong. You shouldn't need to use the + * underlying variable name. Use one of the getVar methods instead. Only use this if you + * are 100% sure you know you're doing. The reason for this is that MetastoreConf goes to a + * lot of trouble to make sure it checks both Hive and Metastore values for config keys. If + * you call conf.get(hivename) you are undermining that. + * @return variable hive name + */ + public String getHiveName() { + return hiveName; + } + @Override public String toString() { return varname; diff --git standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/Metrics.java standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/Metrics.java index c0b1ebc39a..b081026467 100644 --- standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/Metrics.java +++ standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/metrics/Metrics.java @@ -173,15 +173,15 @@ private Metrics(Configuration conf) { */ // Check our config value first. I'm explicitly avoiding getting the default value for now, // as I don't want our default to override a Hive set value. - String reportersToStart = conf.get(MetastoreConf.ConfVars.METRICS_REPORTERS.varname); + String reportersToStart = conf.get(MetastoreConf.ConfVars.METRICS_REPORTERS.getVarname()); if (reportersToStart == null) { // Now look in the current Hive config value. Again, avoiding getting defaults reportersToStart = - conf.get(MetastoreConf.ConfVars.HIVE_CODAHALE_METRICS_REPORTER_CLASSES.hiveName); + conf.get(MetastoreConf.ConfVars.HIVE_CODAHALE_METRICS_REPORTER_CLASSES.getHiveName()); if (reportersToStart == null) { // Last chance, look in the old Hive config value. Still avoiding defaults. reportersToStart = - conf.get(MetastoreConf.ConfVars.HIVE_METRICS_REPORTER.hiveName); + conf.get(MetastoreConf.ConfVars.HIVE_METRICS_REPORTER.getHiveName()); if (reportersToStart == null) { // Alright fine, we'll use our defaults reportersToStart = MetastoreConf.getVar(conf, MetastoreConf.ConfVars.METRICS_REPORTERS); diff --git standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/conf/TestMetastoreConf.java standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/conf/TestMetastoreConf.java index 082b1b0c30..f528e0eabf 100644 --- standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/conf/TestMetastoreConf.java +++ standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/conf/TestMetastoreConf.java @@ -46,7 +46,7 @@ @After public void unsetProperties() { for (MetastoreConf.ConfVars var : MetastoreConf.dataNucleusAndJdoConfs) { - System.getProperties().remove(var.varname); + System.getProperties().remove(var.getVarname()); } } @@ -127,8 +127,8 @@ public void defaults() { Assert.assertTrue(list.contains("c")); Assert.assertSame(TestClass1.class, MetastoreConf.getClass(conf, ConfVars.CLASS_TEST_ENTRY, TestClass1.class, Runnable.class)); - Assert.assertEquals("defaultval", MetastoreConf.get(conf, ConfVars.STR_TEST_ENTRY.varname)); - Assert.assertEquals("defaultval", MetastoreConf.get(conf, ConfVars.STR_TEST_ENTRY.hiveName)); + Assert.assertEquals("defaultval", MetastoreConf.get(conf, ConfVars.STR_TEST_ENTRY.getVarname())); + Assert.assertEquals("defaultval", MetastoreConf.get(conf, ConfVars.STR_TEST_ENTRY.getHiveName())); Assert.assertEquals("defaultval", MetastoreConf.getAsString(conf, ConfVars.STR_TEST_ENTRY)); Assert.assertEquals("42", MetastoreConf.getAsString(conf, ConfVars.LONG_TEST_ENTRY)); Assert.assertEquals("3.141592654", MetastoreConf.getAsString(conf, ConfVars.DOUBLE_TEST_ENTRY)); @@ -162,8 +162,8 @@ public void readMetastoreSiteWithMetastoreConfDir() throws IOException { Assert.assertTrue(list.contains("e")); Assert.assertSame(TestClass2.class, MetastoreConf.getClass(conf, ConfVars.CLASS_TEST_ENTRY, TestClass1.class, Runnable.class)); - Assert.assertEquals("1.8", MetastoreConf.get(conf, ConfVars.DOUBLE_TEST_ENTRY.varname)); - Assert.assertEquals("1.8", MetastoreConf.get(conf, ConfVars.DOUBLE_TEST_ENTRY.hiveName)); + Assert.assertEquals("1.8", MetastoreConf.get(conf, ConfVars.DOUBLE_TEST_ENTRY.getVarname())); + Assert.assertEquals("1.8", MetastoreConf.get(conf, ConfVars.DOUBLE_TEST_ENTRY.getHiveName())); Assert.assertEquals("notthedefault", MetastoreConf.getAsString(conf, ConfVars.STR_TEST_ENTRY)); Assert.assertEquals("37", MetastoreConf.getAsString(conf, ConfVars.LONG_TEST_ENTRY)); Assert.assertEquals("1.8", MetastoreConf.getAsString(conf, ConfVars.DOUBLE_TEST_ENTRY)); @@ -240,11 +240,11 @@ public void setAndRead() throws IOException { @Test public void valuesSetFromProperties() { try { - System.setProperty(MetastoreConf.ConfVars.STR_TEST_ENTRY.varname, "from-properties"); + System.setProperty(MetastoreConf.ConfVars.STR_TEST_ENTRY.getVarname(), "from-properties"); conf = MetastoreConf.newMetastoreConf(); Assert.assertEquals("from-properties", MetastoreConf.getVar(conf, ConfVars.STR_TEST_ENTRY)); } finally { - System.getProperties().remove(MetastoreConf.ConfVars.STR_TEST_ENTRY.varname); + System.getProperties().remove(MetastoreConf.ConfVars.STR_TEST_ENTRY.getVarname()); } } @@ -286,8 +286,8 @@ public void hiveNames() throws IOException { Assert.assertTrue(list.contains("j")); Assert.assertSame(TestClass2.class, MetastoreConf.getClass(conf, ConfVars.CLASS_TEST_ENTRY, TestClass1.class, Runnable.class)); - Assert.assertEquals("3s", MetastoreConf.get(conf, ConfVars.TIME_TEST_ENTRY.varname)); - Assert.assertEquals("3s", MetastoreConf.get(conf, ConfVars.TIME_TEST_ENTRY.hiveName)); + Assert.assertEquals("3s", MetastoreConf.get(conf, ConfVars.TIME_TEST_ENTRY.getVarname())); + Assert.assertEquals("3s", MetastoreConf.get(conf, ConfVars.TIME_TEST_ENTRY.getHiveName())); Assert.assertEquals("hivedefault", MetastoreConf.getAsString(conf, ConfVars.STR_TEST_ENTRY)); Assert.assertEquals("89", MetastoreConf.getAsString(conf, ConfVars.LONG_TEST_ENTRY)); Assert.assertEquals("1.9", MetastoreConf.getAsString(conf, ConfVars.DOUBLE_TEST_ENTRY)); @@ -298,52 +298,52 @@ public void hiveNames() throws IOException { public void timeUnits() throws IOException { conf = MetastoreConf.newMetastoreConf(); - conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.varname, "30s"); + conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.getVarname(), "30s"); Assert.assertEquals(30, MetastoreConf.getTimeVar(conf, ConfVars.TIME_TEST_ENTRY, TimeUnit.SECONDS)); - conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.varname, "30seconds"); + conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.getVarname(), "30seconds"); Assert.assertEquals(30, MetastoreConf.getTimeVar(conf, ConfVars.TIME_TEST_ENTRY, TimeUnit.SECONDS)); - conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.varname, "30ms"); + conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.getVarname(), "30ms"); Assert.assertEquals(30, MetastoreConf.getTimeVar(conf, ConfVars.TIME_TEST_ENTRY, TimeUnit.MILLISECONDS)); - conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.varname, "30msec"); + conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.getVarname(), "30msec"); Assert.assertEquals(30, MetastoreConf.getTimeVar(conf, ConfVars.TIME_TEST_ENTRY, TimeUnit.MILLISECONDS)); - conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.varname, "30us"); + conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.getVarname(), "30us"); Assert.assertEquals(30, MetastoreConf.getTimeVar(conf, ConfVars.TIME_TEST_ENTRY, TimeUnit.MICROSECONDS)); - conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.varname, "30usec"); + conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.getVarname(), "30usec"); Assert.assertEquals(30, MetastoreConf.getTimeVar(conf, ConfVars.TIME_TEST_ENTRY, TimeUnit.MICROSECONDS)); - conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.varname, "30m"); + conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.getVarname(), "30m"); Assert.assertEquals(30, MetastoreConf.getTimeVar(conf, ConfVars.TIME_TEST_ENTRY, TimeUnit.MINUTES)); - conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.varname, "30minutes"); + conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.getVarname(), "30minutes"); Assert.assertEquals(30, MetastoreConf.getTimeVar(conf, ConfVars.TIME_TEST_ENTRY, TimeUnit.MINUTES)); - conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.varname, "30ns"); + conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.getVarname(), "30ns"); Assert.assertEquals(30, MetastoreConf.getTimeVar(conf, ConfVars.TIME_TEST_ENTRY, TimeUnit.NANOSECONDS)); - conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.varname, "30nsec"); + conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.getVarname(), "30nsec"); Assert.assertEquals(30, MetastoreConf.getTimeVar(conf, ConfVars.TIME_TEST_ENTRY, TimeUnit.NANOSECONDS)); - conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.varname, "30h"); + conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.getVarname(), "30h"); Assert.assertEquals(30, MetastoreConf.getTimeVar(conf, ConfVars.TIME_TEST_ENTRY, TimeUnit.HOURS)); - conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.varname, "30hours"); + conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.getVarname(), "30hours"); Assert.assertEquals(30, MetastoreConf.getTimeVar(conf, ConfVars.TIME_TEST_ENTRY, TimeUnit.HOURS)); - conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.varname, "30d"); + conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.getVarname(), "30d"); Assert.assertEquals(30, MetastoreConf.getTimeVar(conf, ConfVars.TIME_TEST_ENTRY, TimeUnit.DAYS)); - conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.varname, "30days"); + conf.set(MetastoreConf.ConfVars.TIME_TEST_ENTRY.getVarname(), "30days"); Assert.assertEquals(30, MetastoreConf.getTimeVar(conf, ConfVars.TIME_TEST_ENTRY, TimeUnit.DAYS)); } @@ -398,9 +398,9 @@ public void timeOutsideExclusive() { @Test public void unprintable() { - Assert.assertTrue(MetastoreConf.isPrintable(ConfVars.STR_TEST_ENTRY.varname)); - Assert.assertFalse(MetastoreConf.isPrintable(ConfVars.PWD.varname)); - Assert.assertFalse(MetastoreConf.isPrintable(ConfVars.PWD.hiveName)); + Assert.assertTrue(MetastoreConf.isPrintable(ConfVars.STR_TEST_ENTRY.getVarname())); + Assert.assertFalse(MetastoreConf.isPrintable(ConfVars.PWD.getVarname())); + Assert.assertFalse(MetastoreConf.isPrintable(ConfVars.PWD.getHiveName())); } @Test diff --git standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/metrics/TestMetrics.java standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/metrics/TestMetrics.java index 259a4db439..70106b9a75 100644 --- standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/metrics/TestMetrics.java +++ standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/metrics/TestMetrics.java @@ -115,7 +115,7 @@ public void allReportersHiveConfig() throws Exception { String jsonFile = System.getProperty("java.io.tmpdir") + System.getProperty("file.separator") + "TestMetricsOutput.json"; Configuration conf = MetastoreConf.newMetastoreConf(); - conf.set(MetastoreConf.ConfVars.HIVE_CODAHALE_METRICS_REPORTER_CLASSES.hiveName, + conf.set(MetastoreConf.ConfVars.HIVE_CODAHALE_METRICS_REPORTER_CLASSES.getHiveName(), "org.apache.hadoop.hive.common.metrics.metrics2.JsonFileMetricsReporter," + "org.apache.hadoop.hive.common.metrics.metrics2.JmxMetricsReporter," + "org.apache.hadoop.hive.common.metrics.metrics2.ConsoleMetricsReporter," + @@ -132,7 +132,7 @@ public void allReportersOldHiveConfig() throws Exception { String jsonFile = System.getProperty("java.io.tmpdir") + System.getProperty("file.separator") + "TestMetricsOutput.json"; Configuration conf = MetastoreConf.newMetastoreConf(); - conf.set(MetastoreConf.ConfVars.HIVE_METRICS_REPORTER.hiveName, + conf.set(MetastoreConf.ConfVars.HIVE_METRICS_REPORTER.getHiveName(), "JSON_FILE,JMX,CONSOLE,HADOOP2"); MetastoreConf.setVar(conf, MetastoreConf.ConfVars.METRICS_JSON_FILE_LOCATION, jsonFile);