You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/01/12 14:21:44 UTC
[GitHub] [hive] zabetak opened a new pull request #1858: HIVE-24569: LLAP daemon leaks file descriptors/log4j appenders
zabetak opened a new pull request #1858:
URL: https://github.com/apache/hive/pull/1858
### What changes were proposed in this pull request?
1. Add Jupiter extension for using mock TCP servers in tests
2. Add Jupiter extension proving LLAP daemons for unit tests
3. Add unit test reproducing the file descriptor leak with LLAP
4. Replace `LlapRoutingAppenderPurgePolicy` with `IdlePurgePolicy`
### Why are the changes needed?
1. Solve the descriptor leak problem.
2. Delegate the burden of managing appenders to the Log4j2 framework
3. Simplify code
4 New extensions are needed for making the unit test reproducer more readable
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
`mvn -pl llap-server test -Dtest=TestLlapDaemonLogging`, fails on f49bbc0dd9a270e7fc6472c43989f6e8f1590e50
`mvn -pl llap-server test -Dtest=TestLlapDaemonLogging`, passes on 458ac1467247f2a7c8275a5ab79a339c63b37e3c
Tests exclusively for the Jupiter extension:
`mvn -pl testutils test -Dtest=TestDoNothingTCPServerExtension`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a change in pull request #1858: HIVE-24569: LLAP daemon leaks file descriptors/log4j appenders
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #1858:
URL: https://github.com/apache/hive/pull/1858#discussion_r563642345
##########
File path: llap-server/src/main/resources/llap-daemon-log4j2.properties
##########
@@ -81,23 +81,21 @@ appender.query-routing.type = Routing
appender.query-routing.name = query-routing
appender.query-routing.routes.type = Routes
appender.query-routing.routes.pattern = $${ctx:queryId}
-#Purge polciy for query-based Routing Appender
-appender.query-routing.purgePolicy.type = LlapRoutingAppenderPurgePolicy
-# Note: Do not change this name without changing the corresponding entry in LlapConstants
-appender.query-routing.purgePolicy.name = llapLogPurgerQueryRouting
+#Purge policy for query-based Routing Appender
+appender.query-routing.purgePolicy.type = IdlePurgePolicy
+appender.query-routing.purgePolicy.timeToLive = 5
Review comment:
@prasanthj I increased the `timeToLive` for production to 60s and made appropriate changes to keep the `.done` when the query completes. I tested the changes on the cluster and things seem to work properly (no descriptor leak, yarn log aggregation working normally). Can you have another look when you find some time?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] prasanthj commented on a change in pull request #1858: HIVE-24569: LLAP daemon leaks file descriptors/log4j appenders
Posted by GitBox <gi...@apache.org>.
prasanthj commented on a change in pull request #1858:
URL: https://github.com/apache/hive/pull/1858#discussion_r563651344
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/log/LlapRandomAccessFileAppender.java
##########
@@ -0,0 +1,185 @@
+/*
+ * 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.hadoop.hive.llap.log;
+
+import org.apache.logging.log4j.core.Appender;
+import org.apache.logging.log4j.core.Core;
+import org.apache.logging.log4j.core.Filter;
+import org.apache.logging.log4j.core.Layout;
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.appender.AbstractOutputStreamAppender;
+import org.apache.logging.log4j.core.appender.RandomAccessFileManager;
+import org.apache.logging.log4j.core.config.Property;
+import org.apache.logging.log4j.core.config.plugins.Plugin;
+import org.apache.logging.log4j.core.config.plugins.PluginBuilderAttribute;
+import org.apache.logging.log4j.core.config.plugins.PluginBuilderFactory;
+
+import java.io.IOException;
+import java.io.Serializable;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+/**
+ * A random access file appender that conditionally renames the file when the appender is stopped.
+ *
+ * If the appender receives an event with a {@link Log4jQueryCompleteMarker} then the underlying file will be remained
Review comment:
nit: remained -> renamed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] prasanthj commented on a change in pull request #1858: HIVE-24569: LLAP daemon leaks file descriptors/log4j appenders
Posted by GitBox <gi...@apache.org>.
prasanthj commented on a change in pull request #1858:
URL: https://github.com/apache/hive/pull/1858#discussion_r558815600
##########
File path: llap-server/src/main/resources/llap-daemon-log4j2.properties
##########
@@ -81,23 +81,21 @@ appender.query-routing.type = Routing
appender.query-routing.name = query-routing
appender.query-routing.routes.type = Routes
appender.query-routing.routes.pattern = $${ctx:queryId}
-#Purge polciy for query-based Routing Appender
-appender.query-routing.purgePolicy.type = LlapRoutingAppenderPurgePolicy
-# Note: Do not change this name without changing the corresponding entry in LlapConstants
-appender.query-routing.purgePolicy.name = llapLogPurgerQueryRouting
+#Purge policy for query-based Routing Appender
+appender.query-routing.purgePolicy.type = IdlePurgePolicy
+appender.query-routing.purgePolicy.timeToLive = 5
Review comment:
Yes. 5 sec is too low. I would keep give it atleast 30 or 60s. Also I remember the log router will rename the files with .done suffix to mark the completion of query and hence the log file. Does it still happen? If we no longer follow the filename convention it will be worth checking if other consumers are expecting files with .done suffix (yarn log aggregation, twz ui)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk merged pull request #1858: HIVE-24569: LLAP daemon leaks file descriptors/log4j appenders
Posted by GitBox <gi...@apache.org>.
kgyrtkirk merged pull request #1858:
URL: https://github.com/apache/hive/pull/1858
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] prasanthj commented on a change in pull request #1858: HIVE-24569: LLAP daemon leaks file descriptors/log4j appenders
Posted by GitBox <gi...@apache.org>.
prasanthj commented on a change in pull request #1858:
URL: https://github.com/apache/hive/pull/1858#discussion_r558482661
##########
File path: llap-server/src/main/resources/llap-daemon-log4j2.properties
##########
@@ -81,23 +81,21 @@ appender.query-routing.type = Routing
appender.query-routing.name = query-routing
appender.query-routing.routes.type = Routes
appender.query-routing.routes.pattern = $${ctx:queryId}
-#Purge polciy for query-based Routing Appender
-appender.query-routing.purgePolicy.type = LlapRoutingAppenderPurgePolicy
-# Note: Do not change this name without changing the corresponding entry in LlapConstants
-appender.query-routing.purgePolicy.name = llapLogPurgerQueryRouting
+#Purge policy for query-based Routing Appender
+appender.query-routing.purgePolicy.type = IdlePurgePolicy
+appender.query-routing.purgePolicy.timeToLive = 5
Review comment:
Question: When does this timer start? after the log file is closed or after every writes? If "idle" is triggered after closing the file I think it is ok. But if it is triggered after the last write then 5 second is probably very low as even if llap is stuck in gc pause it could trigger a file close.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a change in pull request #1858: HIVE-24569: LLAP daemon leaks file descriptors/log4j appenders
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #1858:
URL: https://github.com/apache/hive/pull/1858#discussion_r558673024
##########
File path: llap-server/src/main/resources/llap-daemon-log4j2.properties
##########
@@ -81,23 +81,21 @@ appender.query-routing.type = Routing
appender.query-routing.name = query-routing
appender.query-routing.routes.type = Routes
appender.query-routing.routes.pattern = $${ctx:queryId}
-#Purge polciy for query-based Routing Appender
-appender.query-routing.purgePolicy.type = LlapRoutingAppenderPurgePolicy
-# Note: Do not change this name without changing the corresponding entry in LlapConstants
-appender.query-routing.purgePolicy.name = llapLogPurgerQueryRouting
+#Purge policy for query-based Routing Appender
+appender.query-routing.purgePolicy.type = IdlePurgePolicy
+appender.query-routing.purgePolicy.timeToLive = 5
Review comment:
If 5 sec pass without a write then the appender is deleted and the file is closed. The appender/file will reopen again when logging activity comes back. Do you think this might be a problem?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a change in pull request #1858: HIVE-24569: LLAP daemon leaks file descriptors/log4j appenders
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #1858:
URL: https://github.com/apache/hive/pull/1858#discussion_r563642345
##########
File path: llap-server/src/main/resources/llap-daemon-log4j2.properties
##########
@@ -81,23 +81,21 @@ appender.query-routing.type = Routing
appender.query-routing.name = query-routing
appender.query-routing.routes.type = Routes
appender.query-routing.routes.pattern = $${ctx:queryId}
-#Purge polciy for query-based Routing Appender
-appender.query-routing.purgePolicy.type = LlapRoutingAppenderPurgePolicy
-# Note: Do not change this name without changing the corresponding entry in LlapConstants
-appender.query-routing.purgePolicy.name = llapLogPurgerQueryRouting
+#Purge policy for query-based Routing Appender
+appender.query-routing.purgePolicy.type = IdlePurgePolicy
+appender.query-routing.purgePolicy.timeToLive = 5
Review comment:
@prasanthj I increased the `timeToLive` for production to 60s and made appropriate changes to keep the `.done` when the query completes. I tested the changes on the cluster and things seem to work properly (no descriptor leak, yarn log aggregation working normally). Can you have another look when you find some time?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] prasanthj commented on a change in pull request #1858: HIVE-24569: LLAP daemon leaks file descriptors/log4j appenders
Posted by GitBox <gi...@apache.org>.
prasanthj commented on a change in pull request #1858:
URL: https://github.com/apache/hive/pull/1858#discussion_r563651344
##########
File path: llap-server/src/java/org/apache/hadoop/hive/llap/log/LlapRandomAccessFileAppender.java
##########
@@ -0,0 +1,185 @@
+/*
+ * 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.hadoop.hive.llap.log;
+
+import org.apache.logging.log4j.core.Appender;
+import org.apache.logging.log4j.core.Core;
+import org.apache.logging.log4j.core.Filter;
+import org.apache.logging.log4j.core.Layout;
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.appender.AbstractOutputStreamAppender;
+import org.apache.logging.log4j.core.appender.RandomAccessFileManager;
+import org.apache.logging.log4j.core.config.Property;
+import org.apache.logging.log4j.core.config.plugins.Plugin;
+import org.apache.logging.log4j.core.config.plugins.PluginBuilderAttribute;
+import org.apache.logging.log4j.core.config.plugins.PluginBuilderFactory;
+
+import java.io.IOException;
+import java.io.Serializable;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+/**
+ * A random access file appender that conditionally renames the file when the appender is stopped.
+ *
+ * If the appender receives an event with a {@link Log4jQueryCompleteMarker} then the underlying file will be remained
Review comment:
nit: remained -> renamed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on pull request #1858: HIVE-24569: LLAP daemon leaks file descriptors/log4j appenders
Posted by GitBox <gi...@apache.org>.
zabetak commented on pull request #1858:
URL: https://github.com/apache/hive/pull/1858#issuecomment-758696698
Hey @prasanthj @sidseth , do you have time to review this PR?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org