You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/09/12 12:51:32 UTC
[GitHub] [pulsar] lhotari opened a new pull request, #17589: [fix][common] Fix issue where logs get truncated when Runtime.halt is used to terminate the process
lhotari opened a new pull request, #17589:
URL: https://github.com/apache/pulsar/pull/17589
Fixes #17587
### Motivation
Pulsar log file might not contain the last few log lines when the Pulsar process is abnormally terminated with Runtime.halt.
### Modifications
- Add new utility method `ShutdownUtil.triggerImmediateForcefulShutdown` which calls Log4j2's LogManager.shutdown before terminating the process with Runtime.halt
- Replace usages of `Runtime.halt` with `ShutdownUtil.triggerImmediateForcefulShutdown` in the Pulsar code base
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #17589: [fix][common] Fix issue where logs get truncated when Runtime.halt is used to terminate the process
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #17589:
URL: https://github.com/apache/pulsar/pull/17589#discussion_r970485454
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/ShutdownUtil.java:
##########
@@ -0,0 +1,53 @@
+/**
+ * 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.pulsar.common.util;
+
+import lombok.extern.slf4j.Slf4j;
+import org.apache.logging.log4j.LogManager;
Review Comment:
totally agreed ! good catch
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a diff in pull request #17589: [fix][common] Fix issue where logs get truncated when Runtime.halt is used to terminate the process
Posted by GitBox <gi...@apache.org>.
merlimat commented on code in PR #17589:
URL: https://github.com/apache/pulsar/pull/17589#discussion_r968992981
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/ShutdownUtil.java:
##########
@@ -0,0 +1,53 @@
+/**
+ * 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.pulsar.common.util;
+
+import lombok.extern.slf4j.Slf4j;
+import org.apache.logging.log4j.LogManager;
Review Comment:
I think this puts a dependency on Log4j for the `pulsar-common` module which we shouldn't be having.
Even for brokers, we shouldn't be assuming Log4j is always used.
I think we should be using reflection here to validate that Log4j is in the classpath, instead of using it directly.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #17589: [fix][common] Fix issue where logs get truncated when Runtime.halt is used to terminate the process
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17589:
URL: https://github.com/apache/pulsar/pull/17589#discussion_r973159960
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/ShutdownUtil.java:
##########
@@ -0,0 +1,53 @@
+/**
+ * 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.pulsar.common.util;
+
+import lombok.extern.slf4j.Slf4j;
+import org.apache.logging.log4j.LogManager;
Review Comment:
Fixed
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaozhangmin commented on pull request #17589: [fix][common] Fix issue where logs get truncated when Runtime.halt is used to terminate the process
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on PR #17589:
URL: https://github.com/apache/pulsar/pull/17589#issuecomment-1253268079
@lhotari
With this pr, it looks weird when running pulsar-admin without full params provided.
```
Main parameters are required ("persistent://tenant/namespace/topic")
Get the internal stats for the topic
Usage: stats-internal [options] persistent://tenant/namespace/topic
Options:
-m, --metadata
Flag to include ledger metadata
Default: false
2022-09-21T14:18:44,043+0800 [main] WARN org.apache.pulsar.common.util.ShutdownUtil - Triggering immediate shutdown of current process with status 1
java.lang.Exception: Stacktrace for immediate shutdown
at org.apache.pulsar.common.util.ShutdownUtil.triggerImmediateForcefulShutdown(ShutdownUtil.java:52) ~[org.apache.pulsar-pulsar-common-2.11.0-SNAPSHOT.jar:2.11.0-SNAPSHOT]
at org.apache.pulsar.admin.cli.PulsarAdminTool.exit(PulsarAdminTool.java:309) ~[org.apache.pulsar-pulsar-client-tools-2.11.0-SNAPSHOT.jar:2.11.0-SNAPSHOT]
at org.apache.pulsar.admin.cli.PulsarAdminTool.main(PulsarAdminTool.java:300) ~[org.apache.pulsar-pulsar-client-tools-2.11.0-SNAPSHOT.jar:2.11.0-SNAPSHOT]
```
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #17589: [fix][common] Fix issue where logs get truncated when Runtime.halt is used to terminate the process
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #17589:
URL: https://github.com/apache/pulsar/pull/17589#discussion_r1044167958
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/ShutdownUtil.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * 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.pulsar.common.util;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class ShutdownUtil {
+ private static final Method log4j2ShutdownMethod;
+
+ static {
+ // use reflection to find org.apache.logging.log4j.LogManager.shutdown method
+ Method shutdownMethod = null;
+ try {
+ shutdownMethod = Class.forName("org.apache.logging.log4j.LogManager")
+ .getMethod("shutdown");
+ } catch (ClassNotFoundException | NoSuchMethodException e) {
+ // ignore when Log4j2 isn't found, log at debug level
+ log.debug("Cannot find org.apache.logging.log4j.LogManager.shutdown method", e);
+ }
+ log4j2ShutdownMethod = shutdownMethod;
+ }
+
+
+ /**
+ * Triggers an immediate forceful shutdown of the current process.
+ *
+ * @param status Termination status. By convention, a nonzero status code indicates abnormal termination.
+ * @see Runtime#halt(int)
+ */
+ public static void triggerImmediateForcefulShutdown(int status) {
+ try {
+ if (status != 0) {
+ log.warn("Triggering immediate shutdown of current process with status {}", status,
+ new Exception("Stacktrace for immediate shutdown"));
Review Comment:
@lhotari recently I found that this warning can be annoying or misleading. Said if we run a command `./bin/pulsar-admin schemas delete ` so that it gives the usage info. The user will found:
```
$ ./bin/pulsar-admin schemas delete
Main parameters are required ("persistent://tenant/namespace/topic")
Delete the latest schema for a topic
Usage: delete [options] persistent://tenant/namespace/topic
Options:
-f, --force
whether to delete schema completely. If true, delete all resources
(including metastore and ledger), otherwise only do a mark deletion and
not remove any resources indeed
Default: false
2022-12-09T15:39:22,850+0800 [main] WARN org.apache.pulsar.common.util.ShutdownUtil - Triggering immediate shutdown of current process with status 1
java.lang.Exception: Stacktrace for immediate shutdown
at org.apache.pulsar.common.util.ShutdownUtil.triggerImmediateForcefulShutdown(ShutdownUtil.java:52) ~[pulsar-common.jar:2.11.0-SNAPSHOT]
at org.apache.pulsar.admin.cli.PulsarAdminTool.exit(PulsarAdminTool.java:309) ~[pulsar-client-tools.jar:2.11.0-SNAPSHOT]
at org.apache.pulsar.admin.cli.PulsarAdminTool.main(PulsarAdminTool.java:300) ~[pulsar-client-tools.jar:2.11.0-SNAPSHOT]
```
and confused.
Perhaps we remove this logging line or change it to `debug` level?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #17589: [fix][common] Fix issue where logs get truncated when Runtime.halt is used to terminate the process
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #17589:
URL: https://github.com/apache/pulsar/pull/17589#issuecomment-1249525641
> I agree with @merlimat's comment. Otherwise, LGTM.
@michaeljmarshall @merlimat I addressed the review feedback. PTAL
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari merged pull request #17589: [fix][common] Fix issue where logs get truncated when Runtime.halt is used to terminate the process
Posted by GitBox <gi...@apache.org>.
lhotari merged PR #17589:
URL: https://github.com/apache/pulsar/pull/17589
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org