PARQUET-305: Update logging to SLF4J. #290
Conversation
This removes the Log implementation based on java.util.logging and replaces it with SLF4J. The compiler removal of debug log messages still works because Log.DEBUG and similar final constants are unchanged. The Log class and API is now deprecated in favor of using SLF4J directly. The main reason is to avoid expensive string construction when possible by exposing the full SLF4J API. This commit adds slf4j-simple as the test logger implementation. Configuration for slf4j-simple is in the root pom. Two modules can't use slf4j-simple, parquet-pig and parquet-thrift, and use slf4j-log4j12 instead because pig depends on log4j and tests die without it.
|
@julienledem, as long as I was changing the logging code to fix PARQUET-305, I went ahead and switched it over to SLF4J. Could you have a look? I marked the |
|
Thanks for updating the dependency and use slf4j under the hood. |
|
The problem with not moving to the slf4j API is that it forces the caller to do a lot of string manipulation to construct a log message regardless of whether it is actually used. The varargs style API defers constructing the log message. I also don't see much value in having a wrapper around the logging implementation, since that is what slf4j is supposed to do. That API already allows the downstream application to switch out the logger implementation used by Parquet. My reasoning is: why maintain our own API when it results in not being able to use features of the underlying one that is already general-purpose? |
There was a problem hiding this comment.
I'd rather not make this deprecated in this PR since it will make every class have warnings.
Otherwise, this looks good to me.
There was a problem hiding this comment.
No problem. I'll open a follow-up issue for further discussion. We can deprecate and remove the use of this class for that issue instead of dragging out this one.
|
+1 |
|
hi @rdblue, I wonder if there is any following-up discussion/JIRA/PR concerning replacing the use of Log.java with slf4j? Maybe I can do the replacing, should we reach consensus. |
|
I don't recall there's a jira already. If not please make one so that we can have the discussion. |
|
I opened an issue to move over to SLF4J already. It is: https://issues.apache.org/jira/browse/PARQUET-401 |
This removes the Log implementation based on java.util.logging and replaces it with SLF4J. The compiler removal of debug log messages still works because Log.DEBUG and similar final constants are unchanged. This commit adds slf4j-simple as the test logger implementation. Configuration for slf4j-simple is in the root pom. Two modules can't use slf4j-simple, parquet-pig and parquet-thrift, and use slf4j-log4j12 instead because pig depends on log4j and tests die without it. Author: Ryan Blue <blue@apache.org> Closes apache#290 from rdblue/PARQUET-305-update-logging and squashes the following commits: 89257e8 [Ryan Blue] PARQUET-305: Remove deprecation annotations on Log. 9f9b99a [Ryan Blue] PARQUET-305: Update logging to SLF4J.
This removes the Log implementation based on java.util.logging and replaces it with SLF4J. The compiler removal of debug log messages still works because Log.DEBUG and similar final constants are unchanged. This commit adds slf4j-simple as the test logger implementation. Configuration for slf4j-simple is in the root pom. Two modules can't use slf4j-simple, parquet-pig and parquet-thrift, and use slf4j-log4j12 instead because pig depends on log4j and tests die without it. Author: Ryan Blue <blue@apache.org> Closes apache#290 from rdblue/PARQUET-305-update-logging and squashes the following commits: 89257e8 [Ryan Blue] PARQUET-305: Remove deprecation annotations on Log. 9f9b99a [Ryan Blue] PARQUET-305: Update logging to SLF4J.
This removes the Log implementation based on java.util.logging and replaces it with SLF4J. The compiler removal of debug log messages still works because Log.DEBUG and similar final constants are unchanged. This commit adds slf4j-simple as the test logger implementation. Configuration for slf4j-simple is in the root pom. Two modules can't use slf4j-simple, parquet-pig and parquet-thrift, and use slf4j-log4j12 instead because pig depends on log4j and tests die without it. Author: Ryan Blue <blue@apache.org> Closes apache#290 from rdblue/PARQUET-305-update-logging and squashes the following commits: 89257e8 [Ryan Blue] PARQUET-305: Remove deprecation annotations on Log. 9f9b99a [Ryan Blue] PARQUET-305: Update logging to SLF4J.
3 participants
This removes the Log implementation based on java.util.logging and
replaces it with SLF4J. The compiler removal of debug log messages still
works because Log.DEBUG and similar final constants are unchanged.
This commit adds slf4j-simple as the test logger implementation.
Configuration for slf4j-simple is in the root pom. Two modules can't use
slf4j-simple, parquet-pig and parquet-thrift, and use slf4j-log4j12
instead because pig depends on log4j and tests die without it.