Log message when Tablet has been unloading for over 15 minutes - #4558
Conversation
Was having an issue in PR apache#4488 with an earlier version of the code complaining about Serializable. This seems to be resolved in this version of the code.
cshannon
left a comment
cshannon
left a comment
There was a problem hiding this comment.
I left a few comments, primarily around how the cache is implemented and used I think should be changed. But the other big thing is I am thinking this custom logic would be better implemented using the Filter API that is part of log4j2. I think we could create a custom filter and put the conditional logic in there instead of having to implement a new ConditionalLogger class. The nice thing about a filter too is it is configurable at runtime easily.
But we are coding to the slf4j api. A user using log4j2 is a deployment decision, someone could swap it out for logback or something else. |
That's a fair point, I'm not sure it's very likely someone would replace that too often but it's possible. I guess in that guess it wouldn't break per se, but it would just start logging a lot of extra stuff of course. |
|
The changes here caused a compilation issue in my dev environment because apparently log4j-api is a transitive dependency of log4j-1.2-api and is needed indirectly because some of the classes that Category requires are in log4j-api and the 1.2 Logger class extends Category. So, by adding an entry for log4j-api for the test scope, it was prevented from being added transitively to the compile scope for log4j-1.2-api. It did work in some environments, though... because log4j-api leaks into the compile time classpath from a lot of places (including Maven itself), but it's not strictly correct, and does break the build in some environments. I'm working on a fix for this, as well as some other cleanups. |
* Bump commons-logging to 1.3, so it natively supports sending logs to slf4j or log4j without the log4j-jcl bridge See apache/commons-logging#177 * Fix issue described in apache#4558 (comment) regarding log4j-api being required for log4j-1.2-api in core/pom.xml * Fix some false-positive Closeable resource warnings from wrapping a Scanner with an IsolatedScanner * Suppress some deprecation warnings for scan server metadata schema * Remove log4j-1.2-api from minicluster and compactor modules, where they weren't used * Fix some incorrect `@SuppressWarnings` comments * Fix generics with the ServiceStatusCmd's Result class, which had a generic `A extends Integer` parameter, but `Integer` is final and cannot be extended * Remove some unused variables * Add a few comments in the POMs to explain why some log4j dependencies exist where they do * Add a try-with-resources block in ClientSideIteratorIT to ensure the scanner is closed when done
* Bump commons-logging to 1.3, so it natively supports sending logs to slf4j or log4j without the log4j-jcl bridge See apache/commons-logging#177 * Fix issue described in apache#4558 (comment) regarding log4j-api being required for log4j-1.2-api in core/pom.xml by making sure bnd and log4j-api are in the compile scope but marked as provided to avoid the maven-dependency-plugin from complaining because they appear to be required only for the test, but actually are still needed transitively for log4j-1.2-api * Fix some false-positive Closeable resource warnings from wrapping a Scanner with an IsolatedScanner * Suppress some deprecation warnings for scan server metadata schema * Remove log4j-1.2-api from minicluster and compactor modules, where they weren't used * Fix some incorrect `@SuppressWarnings` comments * Fix generics with the ServiceStatusCmd's Result class, which had a generic `A extends Integer` parameter, but `Integer` is final and cannot be extended * Remove some unused variables * Add a few comments in the POMs to explain why some log4j dependencies exist where they do * Add a try-with-resources block in ClientSideIteratorIT to ensure the scanner is closed when done * Remove an unneeded exclusion on httpclient from libthrift (it's not a transitive dependency in the POM for the version of libthrift we are using, and upstream they have already migrated to newer versions of httpclient5 in the latest POM for libthrift)
* Bump commons-logging to 1.3, so it natively supports sending logs to slf4j or log4j without the log4j-jcl bridge See apache/commons-logging#177 * Fix issue described in #4558 (comment) regarding log4j-api being required for log4j-1.2-api in core/pom.xml by making sure bnd and log4j-api are in the compile scope but marked as provided to avoid the maven-dependency-plugin from complaining because they appear to be required only for the test, but actually are still needed transitively for log4j-1.2-api * Fix some false-positive Closeable resource warnings from wrapping a Scanner with an IsolatedScanner * Suppress some deprecation warnings for scan server metadata schema * Remove log4j-1.2-api from minicluster and compactor modules, where they weren't used * Fix some incorrect `@SuppressWarnings` comments * Fix generics with the ServiceStatusCmd's Result class, which had a generic `A extends Integer` parameter, but `Integer` is final and cannot be extended * Remove some unused variables * Add a few comments in the POMs to explain why some log4j dependencies exist where they do * Add a try-with-resources block in ClientSideIteratorIT to ensure the scanner is closed when done * Remove an unneeded exclusion on httpclient from libthrift (it's not a transitive dependency in the POM for the version of libthrift we are using, and upstream they have already migrated to newer versions of httpclient5 in the latest POM for libthrift)
* Remove log4j-1.2-api dependency except as a bridge in the binary assembly (left a note in the assemble/pom.xml) * Move log4j dependencies in core back to test scope. These were moved to the provided scope in apache#4743 to work around an issue because log4j-1.2-api was still used in the core module in 2.1; now that the legacy log4j stuff was removed from core in 3.1 and later, we can move these back to the test scope where they belong as apache#4558 intended * Specify the exact version for deprecation "since" parameters (3.1.0, instead of 3.1) * Drop the unused and deprecated `tserver.workq.threads` property * Remove a few unused imports and variables, and drop the legacy MeterRegistryFactory that was deprecated in 2.1.3 * Suppress some deprecation warnings and use var in a few places for readability * Delete unused test code in CompactionIT
* Cleanup of main branch * Remove log4j-1.2-api dependency except as a bridge in the binary assembly (left a note in the assemble/pom.xml), and as a runtime dependency of hadoop minicluster, in the proper scope where needed * Move log4j dependencies in core back to test scope. These were moved to the provided scope in #4743 to work around an issue because log4j-1.2-api was still used in the core module in 2.1; now that the legacy log4j stuff was removed from core in 3.1 and later, we can move these back to the test scope where they belong as #4558 intended * Specify the exact version for deprecation "since" parameters (3.1.0, instead of 3.1) * Drop the unused and deprecated `tserver.workq.threads` property * Remove a few unused imports and variables, and drop the legacy MeterRegistryFactory that was deprecated in 2.1.3 * Suppress some deprecation warnings and use var in a few places for readability * Delete unused test code in CompactionIT
4 participants
Related to #4539