ayushtkn commented on code in PR #5520: URL: https://github.com/apache/hive/pull/5520#discussion_r1830458552
########## itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestExceptionMessageWithJdbc.java: ########## @@ -0,0 +1,157 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hive.jdbc; + +import org.apache.hadoop.hive.conf.HiveConf; +import org.apache.hadoop.hive.conf.HiveConf.ConfVars; +import org.apache.hadoop.hive.conf.HiveConfForTest; +import org.apache.hadoop.hive.ql.ServiceContext; +import org.apache.hive.jdbc.miniHS2.MiniHS2; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.io.IOException; +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +public class TestExceptionMessageWithJdbc { + private static MiniHS2 miniHS2; + private static Connection conDefault; + private static Connection conTestDb; + private static final String TEST_DB_NAME = "test_logging_level"; + private static final String USERNAME = System.getProperty("user.name"); + private static final String PASSWORD = "bar"; + + @BeforeClass + public static void setupBeforeClass() throws Exception { + MiniHS2.cleanupLocalDir(); + HiveConf conf = getNewHiveConf(); + try { + startMiniHS2(conf); + } catch (Exception e) { + System.out.println("Unable to start MiniHS2: " + e); + throw e; + } + + try { + openDefaultConnections(); + } catch (Exception e) { + System.out.println("Unable to open default connections to MiniHS2: " + e); + throw e; + } + Statement stmt = conDefault.createStatement(); + stmt.execute("drop database if exists " + TEST_DB_NAME + " cascade"); + stmt.execute("create database " + TEST_DB_NAME); + stmt.close(); + + try { + openTestConnections(); + } catch (Exception e) { + System.out.println("Unable to open default connections to MiniHS2: " + e); + throw e; + } + } + + private static Connection getConnection() throws Exception { + return getConnection(miniHS2.getJdbcURL(), USERNAME, PASSWORD); + } + + private static Connection getConnection(String dbName) throws Exception { + return getConnection(miniHS2.getJdbcURL(dbName), USERNAME, PASSWORD); + } + + private static Connection getConnection(String jdbcURL, String user, String pwd) + throws SQLException { + Connection conn = DriverManager.getConnection(jdbcURL, user, pwd); + assertNotNull(conn); + return conn; + } + + @AfterClass + public static void tearDownAfterClass() throws Exception { + Statement stmt = conDefault.createStatement(); + stmt.execute("set hive.support.concurrency = false"); + stmt.execute("drop database if exists " + TEST_DB_NAME + " cascade"); + stmt.close(); + if (conTestDb != null) { + conTestDb.close(); + } + if (conDefault != null) { + conDefault.close(); + } + stopMiniHS2(); + cleanupMiniHS2(); + } + + private static void startMiniHS2(HiveConf conf) throws Exception { + MiniHS2.Builder builder = new MiniHS2.Builder().withConf(conf).cleanupLocalDirOnStartup(false); + miniHS2 = builder.build(); + + Map<String, String> confOverlay = new HashMap<>(); + miniHS2.start(confOverlay); + } + + private static void stopMiniHS2() { + if ((miniHS2 != null) && (miniHS2.isStarted())) { + miniHS2.stop(); + } + } + + private static void cleanupMiniHS2() throws IOException { + if (miniHS2 != null) { + miniHS2.cleanup(); + } + MiniHS2.cleanupLocalDir(); + } + + private static void openDefaultConnections() throws Exception { + conDefault = getConnection(); + } + + private static void openTestConnections() throws Exception { + conTestDb = getConnection(TEST_DB_NAME); + } + + private static HiveConf getNewHiveConf() { + HiveConf conf = new HiveConfForTest(TestExceptionMessageWithJdbc.class); + conf.setVar(ConfVars.HIVE_EXECUTION_ENGINE, "mr"); Review Comment: Why are we running the tests with MR, MR is deprecated we should avoid adding new tests with it ########## ql/src/java/org/apache/hadoop/hive/ql/ServiceContext.java: ########## @@ -30,6 +30,7 @@ public class ServiceContext { private final String clusterId; private final Supplier<String> host; private final Supplier<Integer> port; + public static final String QUERY_ID = "Query ID"; Review Comment: I am not sure if this is the correct place to keep this variable, The Javadoc of ``ServiceContext`` itself says ``Information context about the service, most probably HiveServer2.`` ########## service/src/java/org/apache/hive/service/cli/HiveSQLException.java: ########## @@ -150,4 +191,13 @@ public static TStatus toTStatus(Exception e) { return tStatus; } + @Override + public String getMessage(){ + String errorMsg = super.getMessage() == null ? "Error" : super.getMessage(); + if (!errorMsg.contains(ServiceContext.QUERY_ID)) { + return String.format("%s; Query ID: %s", errorMsg, queryId); Review Comment: Shouldn't we check ``queryId`` not null as well? else someone having an old jar which creates this HiveSqlException without QueryId will start getting this with null? ########## service/src/java/org/apache/hive/service/cli/HiveSQLException.java: ########## @@ -70,6 +72,11 @@ public HiveSQLException(Throwable cause) { super(cause); } + public HiveSQLException(Throwable cause, String queryId) { Review Comment: We should even deprecate the old constructors without queryId & remove in the subsequent release ########## service/src/java/org/apache/hive/service/ServiceUtils.java: ########## @@ -72,5 +72,4 @@ public static boolean canProvideProgressLog(HiveConf hiveConf) { return "tez".equals(hiveConf.getVar(HiveConf.ConfVars.HIVE_EXECUTION_ENGINE)) && hiveConf .getBoolVar(HiveConf.ConfVars.HIVE_SERVER2_INPLACE_PROGRESS); } - Review Comment: nit avoid ########## service/src/java/org/apache/hive/service/cli/HiveSQLException.java: ########## @@ -150,4 +191,13 @@ public static TStatus toTStatus(Exception e) { return tStatus; } + @Override + public String getMessage(){ + String errorMsg = super.getMessage() == null ? "Error" : super.getMessage(); + if (!errorMsg.contains(ServiceContext.QUERY_ID)) { Review Comment: curious: is the contains to avoid appending queryId multiple times, there could be cases where query id is already appended & we want to avoid appending it again? Something like that? ########## service/src/java/org/apache/hive/service/cli/operation/Operation.java: ########## @@ -367,13 +367,13 @@ protected void validateFetchOrientation(FetchOrientation orientation, EnumSet<FetchOrientation> supportedOrientations) throws HiveSQLException { if (!supportedOrientations.contains(orientation)) { throw new HiveSQLException("The fetch type " + orientation.toString() + - " is not supported for this resultset", "HY106"); + " is not supported for this resultset", "HY106", queryState.getQueryId()); } } - protected HiveSQLException toSQLException(String prefix, CommandProcessorException e) { + protected HiveSQLException toSQLException(String prefix, CommandProcessorException e, QueryState queryState) { Review Comment: Why are you having ``queryState`` as a parameter & then extracting ``queryId`` from it, we can directly have ``queryId`` as a parameter? as in case of constructor of ``HiveSQLException`` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
