diff --git a/common/src/java/org/apache/hadoop/hive/common/LogUtils.java b/common/src/java/org/apache/hadoop/hive/common/LogUtils.java index 5c7ec69f04587ab18704a10fade6e3a47601a87f..8fdba02f07e780a443add8755961c641abbf957e 100644 --- a/common/src/java/org/apache/hadoop/hive/common/LogUtils.java +++ b/common/src/java/org/apache/hadoop/hive/common/LogUtils.java @@ -248,14 +248,9 @@ public static void stopQueryAppender(String routingAppenderName, String queryId) // The appender is configured to use ${ctx:queryId} by registerRoutingAppender() try { Class clazz = routingAppender.getClass(); - Method method = clazz.getDeclaredMethod("getControl", String.class, LogEvent.class); + Method method = clazz.getDeclaredMethod("deleteAppender", String.class); method.setAccessible(true); - AppenderControl control = (AppenderControl) method.invoke(routingAppender, queryId, null); - Appender subordinateAppender = control.getAppender(); - if (!subordinateAppender.isStopped()) { - // this will cause the subordinate appender to close its output stream. - subordinateAppender.stop(); - } + method.invoke(routingAppender, queryId); } catch (NoSuchMethodException | SecurityException | IllegalAccessException | IllegalArgumentException | InvocationTargetException e) { l4j.warn("Unable to close the operation log appender for query id " + queryId, e); diff --git a/itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java b/itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java index 8febe3e79ff892c54b696b6c6ef92f7026c46033..faf78c73fff4a5a69b0e71309ca2e59797bd285f 100644 --- a/itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java +++ b/itests/hive-unit/src/test/java/org/apache/hive/service/cli/operation/TestOperationLoggingLayout.java @@ -129,9 +129,61 @@ public void testSwitchLogLayout() throws Exception { client.closeOperation(operationHandle); checkAppenderState("after operation close ", LogDivertAppender.QUERY_ROUTING_APPENDER, queryId, true); checkAppenderState("after operation close ", LogDivertAppenderForTest.TEST_QUERY_ROUTING_APPENDER, queryId, true); - } + @Test + /** + * Test to make sure that appending log event to HushableRandomAccessFileAppender even after + * closing the corresponding operation would not throw an exception. + */ + public void testHushableRandomAccessFileAppender() throws Exception { + // verify whether the sql operation log is generated and fetch correctly. + OperationHandle operationHandle = client.executeStatement(sessionHandle, sqlCntStar, null); + RowSet rowSetLog = client.fetchResults(operationHandle, FetchOrientation.FETCH_FIRST, 1000, + FetchType.LOG); + Iterator iter = rowSetLog.iterator(); + String queryId = null; + Appender queryAppender = null; + Appender testQueryAppender = null; + HushableRandomAccessFileAppender hushableAppender = null; + // non-verbose pattern is %-5p : %m%n. Look for " : " + while (iter.hasNext()) { + String row = iter.next()[0].toString(); + Assert.assertEquals(true, row.matches("^.*(FATAL|ERROR|WARN|INFO|DEBUG|TRACE).*$")); + + // look for a row like "INFO : Query ID = asherman_20170718154720_17c7d18b-36e6-4b35-a8e2-f50847db58ae" + String queryIdLoggingProbe = "INFO : Query ID = "; + int index = row.indexOf(queryIdLoggingProbe); + if (index >= 0) { + queryId = row.substring(queryIdLoggingProbe.length()).trim(); + } + } + Assert.assertNotNull("Could not find query id, perhaps a logging message changed", queryId); + + checkAppenderState("before operation close ", LogDivertAppender.QUERY_ROUTING_APPENDER, queryId, false); + queryAppender = getAppender(LogDivertAppender.QUERY_ROUTING_APPENDER, queryId); + checkAppenderState("before operation close ", LogDivertAppenderForTest.TEST_QUERY_ROUTING_APPENDER, queryId, false); + testQueryAppender = getAppender(LogDivertAppenderForTest.TEST_QUERY_ROUTING_APPENDER, queryId); + + client.closeOperation(operationHandle); + if((queryAppender!= null) && queryAppender instanceof HushableRandomAccessFileAppender) { + hushableAppender = (HushableRandomAccessFileAppender) queryAppender; + try { + hushableAppender.append(new LocalLogEvent()); + } catch (Exception e) { + Assert.fail("Exception is not expected while appending HushableRandomAccessFileAppender"); + } + } + + if((testQueryAppender!= null) && testQueryAppender instanceof HushableRandomAccessFileAppender) { + hushableAppender = (HushableRandomAccessFileAppender) testQueryAppender; + try { + hushableAppender.append(new LocalLogEvent()); + } catch (Exception e) { + Assert.fail("Exception is not expected while appending HushableRandomAccessFileAppender"); + } + } + } /** * assert that the appender for the given queryId is in the expected state * @param msg a diagnostic @@ -151,19 +203,40 @@ private void checkAppenderState(String msg, String routingAppenderName, String q defaultsField.setAccessible(true); ConcurrentHashMap appenders = (ConcurrentHashMap) defaultsField.get(routingAppender); AppenderControl appenderControl = (AppenderControl) appenders.get(queryId); - Assert.assertNotNull(msg + "could not find AppenderControl for query id " + queryId, appenderControl); - Appender appender = appenderControl.getAppender(); - Assert.assertNotNull(msg + "could not find Appender for query id " + queryId + " from AppenderControl " + appenderControl, appender); - Assert.assertEquals(msg + "Appender for query is in unexpected state", expectedStopped, appender.isStopped()); - Assert.assertTrue("Appender should be a HushableMutableRandomAccessAppender", appender instanceof HushableRandomAccessFileAppender); - HushableRandomAccessFileAppender ra = (HushableRandomAccessFileAppender) appender; - // Even if the appender is stopped it should not throw an exception when we log - try { - ra.append(new LocalLogEvent()); - } catch (Exception e) { - e.printStackTrace(); - Assert.fail("Caught exception while logging to an appender of class " + ra.getClass() - + " with stopped=" + ra.isStopped()); + if (!expectedStopped) { + Assert.assertNotNull(msg + "Could not find AppenderControl for query id " + queryId, appenderControl); + Appender appender = appenderControl.getAppender(); + Assert.assertNotNull(msg + "could not find Appender for query id " + queryId + " from AppenderControl " + appenderControl, appender); + Assert.assertEquals(msg + "Appender for query is in unexpected state", expectedStopped, appender.isStopped()); + } else { + Assert.assertNull(msg + "AppenderControl for query id is not removed" + queryId, appenderControl); + } + } + + /** + * Get the appender associated with a query + * @param routingAppenderName Routing appender name + * @param queryId Query Id for the operation + * @return Appender if found, else null + * @throws NoSuchFieldException + * @throws IllegalAccessException + */ + private Appender getAppender(String routingAppenderName, String queryId) + throws NoSuchFieldException, IllegalAccessException { + LoggerContext context = (LoggerContext) LogManager.getContext(false); + Configuration configuration = context.getConfiguration(); + LoggerConfig loggerConfig = configuration.getRootLogger(); + Map appendersMap = loggerConfig.getAppenders(); + RoutingAppender routingAppender = (RoutingAppender) appendersMap.get(routingAppenderName); + Assert.assertNotNull("could not find routingAppender " + routingAppenderName, routingAppender); + Field defaultsField = RoutingAppender.class.getDeclaredField("appenders"); + defaultsField.setAccessible(true); + ConcurrentHashMap appenders = (ConcurrentHashMap) defaultsField.get(routingAppender); + AppenderControl appenderControl = (AppenderControl) appenders.get(queryId); + if(appenderControl != null) { + return appenderControl.getAppender(); + } else { + return null; } }