You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fury.apache.org by "chaokunyang (via GitHub)" <gi...@apache.org> on 2024/04/09 13:17:24 UTC
[PR] feat(java): implement fury logger and remove slf4j library [incubator-fury]
chaokunyang opened a new pull request, #1485:
URL: https://github.com/apache/incubator-fury/pull/1485
This PR implemented fury logger and remove slf4j library
Closes #1484
Closes #1404
--
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@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(java): implement fury logger and remove slf4j library [incubator-fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on PR #1485:
URL: https://github.com/apache/incubator-fury/pull/1485#issuecomment-2045512782
> We cannot guarantee that `Log.debug` will not be used. If `Log.debug` is used in some frequently executed locations, a large amount of logs will be output because we have no level control.
>
> Maybe we can open an Issue and add the `good first use` tag to provide training opportunities for new developers who want to join the Fury community. This need is not very urgent for us.
The debug log should be avoided in critical path. But I agree with you that we can provide log level for reduce info and other logs print.
I prefer not to provide debug api in fury Logger. Without this API, users won't print debug log.
The log level api has its cost too. Jvm can't eliminate the condition checks, you can see how kryo use a static final constant to reduce the log cost
--
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@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(java): implement fury logger and remove slf4j library [incubator-fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on PR #1485:
URL: https://github.com/apache/incubator-fury/pull/1485#issuecomment-2046382532
> > But I agree with you that we can provide log level for reduce info and other logs print.
>
> Yes, we can control the output of other levels of Log.
>
> > The log level api has its cost too. Jvm can't eliminate the condition checks, you can see how kryo use a static final constant to reduce the log cost
>
> FuryLogger is only used internally. We can add a `static final` Level field in FuryLogger to control the output Log level. In this way, Log Level does not support dynamic setting.
Nice, looks good to me.
--
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@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(java): implement fury logger and remove slf4j library [incubator-fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on PR #1485:
URL: https://github.com/apache/incubator-fury/pull/1485#issuecomment-2045293452
> Don't we need to control the log level of printing? I see that all current levels of logs can be printed.
This is for Fury internal use. If users need such control, he can use slf4j. Such support in Fury is better but not a strong requirement. Maybe we can support it in future
--
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@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(java): implement fury logger and remove slf4j library [incubator-fury]
Posted by "LiangliangSui (via GitHub)" <gi...@apache.org>.
LiangliangSui commented on PR #1485:
URL: https://github.com/apache/incubator-fury/pull/1485#issuecomment-2045222355
Don't we need to control the log level of printing? I see that all current levels of logs can be printed.
--
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@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(java): implement fury logger and remove slf4j library [incubator-fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang merged PR #1485:
URL: https://github.com/apache/incubator-fury/pull/1485
--
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@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(java): implement fury logger and remove slf4j library [incubator-fury]
Posted by "LiangliangSui (via GitHub)" <gi...@apache.org>.
LiangliangSui commented on PR #1485:
URL: https://github.com/apache/incubator-fury/pull/1485#issuecomment-2045618888
> But I agree with you that we can provide log level for reduce info and other logs print.
Yes, we can control the output of other levels of Log.
> The log level api has its cost too. Jvm can't eliminate the condition checks, you can see how kryo use a static final constant to reduce the log cost
FuryLogger is only used internally. We can add a `static final` Level field in FuryLogger to control the output Log level. In this way, Log Level does not support dynamic setting, which saves the overhead of JVM condition checking.
--
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@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(java): implement fury logger and remove slf4j library [incubator-fury]
Posted by "LiangliangSui (via GitHub)" <gi...@apache.org>.
LiangliangSui commented on code in PR #1485:
URL: https://github.com/apache/incubator-fury/pull/1485#discussion_r1557659445
##########
java/fury-core/src/main/java/org/apache/fury/logging/FuryLogger.java:
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.fury.logging;
+
+import java.time.LocalDateTime;
+import java.time.format.DateTimeFormatter;
+
+public class FuryLogger implements Logger {
+ private static final DateTimeFormatter dateTimeFormatter =
+ DateTimeFormatter.ofPattern("yyyy-MM-dd hh:mm:ss");
+ private final String name;
+
+ public FuryLogger(Class<?> targetClass) {
+ this.name = targetClass.getSimpleName();
+ }
+
+ // The implementation should not forward to other method, otherwise the fileNumber won't be right.
+ @Override
+ public void info(String msg) {
+ log("INFO", msg, new Object[0], false);
+ }
+
+ @Override
+ public void info(String msg, Object arg) {
+ log("INFO", msg, new Object[] {arg}, false);
+ }
+
+ @Override
+ public void info(String msg, Object arg1, Object arg2) {
+ log("INFO", msg, new Object[] {arg1, arg2}, false);
+ }
+
+ @Override
+ public void info(String msg, Object... args) {
+ log("INFO", msg, args, false);
+ }
+
+ @Override
+ public void warn(String msg) {
+ log("WARN", msg, new Object[0], false);
+ }
+
+ @Override
+ public void warn(String msg, Object arg) {
+ log("WARN", msg, new Object[] {arg}, true);
+ }
+
+ @Override
+ public void warn(String msg, Object arg1, Object arg2) {
+ log("WARN", msg, new Object[] {arg1, arg2}, true);
+ }
+
+ @Override
+ public void warn(String msg, Object... args) {
+ log("WARN", msg, args, true);
+ }
+
+ @Override
+ public void warn(String msg, Throwable t) {
+ log("WARN", msg, new Object[] {t}, true);
+ }
+
+ @Override
+ public void error(String msg) {
+ log("ERROR", msg, new Object[0], false);
+ }
+
+ @Override
+ public void error(String msg, Object arg) {
+ log("ERROR", msg, new Object[] {arg}, true);
+ }
+
+ @Override
+ public void error(String msg, Object arg1, Object arg2) {
+ log("ERROR", msg, new Object[] {arg1, arg2}, true);
+ }
+
+ @Override
+ public void error(String msg, Object... args) {
+ log("ERROR", msg, args, true);
+ }
+
+ @Override
+ public void error(String msg, Throwable t) {
+ log("ERROR", msg, new Object[] {t}, true);
+ }
+
+ private void log(String level, String msg, Object[] args, boolean mayPrintTrace) {
Review Comment:
Could we replace `String level` with a `Level enum`?
--
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@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(java): implement fury logger and remove slf4j library [incubator-fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on code in PR #1485:
URL: https://github.com/apache/incubator-fury/pull/1485#discussion_r1557720365
##########
java/fury-core/src/main/java/org/apache/fury/logging/FuryLogger.java:
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.fury.logging;
+
+import java.time.LocalDateTime;
+import java.time.format.DateTimeFormatter;
+
+public class FuryLogger implements Logger {
+ private static final DateTimeFormatter dateTimeFormatter =
+ DateTimeFormatter.ofPattern("yyyy-MM-dd hh:mm:ss");
+ private final String name;
+
+ public FuryLogger(Class<?> targetClass) {
+ this.name = targetClass.getSimpleName();
+ }
+
+ // The implementation should not forward to other method, otherwise the fileNumber won't be right.
+ @Override
+ public void info(String msg) {
+ log("INFO", msg, new Object[0], false);
+ }
+
+ @Override
+ public void info(String msg, Object arg) {
+ log("INFO", msg, new Object[] {arg}, false);
+ }
+
+ @Override
+ public void info(String msg, Object arg1, Object arg2) {
+ log("INFO", msg, new Object[] {arg1, arg2}, false);
+ }
+
+ @Override
+ public void info(String msg, Object... args) {
+ log("INFO", msg, args, false);
+ }
+
+ @Override
+ public void warn(String msg) {
+ log("WARN", msg, new Object[0], false);
+ }
+
+ @Override
+ public void warn(String msg, Object arg) {
+ log("WARN", msg, new Object[] {arg}, true);
+ }
+
+ @Override
+ public void warn(String msg, Object arg1, Object arg2) {
+ log("WARN", msg, new Object[] {arg1, arg2}, true);
+ }
+
+ @Override
+ public void warn(String msg, Object... args) {
+ log("WARN", msg, args, true);
+ }
+
+ @Override
+ public void warn(String msg, Throwable t) {
+ log("WARN", msg, new Object[] {t}, true);
+ }
+
+ @Override
+ public void error(String msg) {
+ log("ERROR", msg, new Object[0], false);
+ }
+
+ @Override
+ public void error(String msg, Object arg) {
+ log("ERROR", msg, new Object[] {arg}, true);
+ }
+
+ @Override
+ public void error(String msg, Object arg1, Object arg2) {
+ log("ERROR", msg, new Object[] {arg1, arg2}, true);
+ }
+
+ @Override
+ public void error(String msg, Object... args) {
+ log("ERROR", msg, args, true);
+ }
+
+ @Override
+ public void error(String msg, Throwable t) {
+ log("ERROR", msg, new Object[] {t}, true);
+ }
+
+ private void log(String level, String msg, Object[] args, boolean mayPrintTrace) {
Review Comment:
Good point
--
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@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [PR] feat(java): implement fury logger and remove slf4j library [incubator-fury]
Posted by "LiangliangSui (via GitHub)" <gi...@apache.org>.
LiangliangSui commented on PR #1485:
URL: https://github.com/apache/incubator-fury/pull/1485#issuecomment-2045412314
We cannot guarantee that `Log.debug` will not be used. If `Log.debug` is used in some frequently executed locations, a large amount of logs will be output because we have no level control.
Maybe we can open an Issue and add the `good first use` tag to provide training opportunities for new developers who want to join the Fury community. This need is not very urgent for us.
--
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@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org