-
Notifications
You must be signed in to change notification settings - Fork 453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
core/src/main/java/org/apache/accumulo/core/logging/ConditionalLogger.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/accumulo/core/logging/ConditionalLogger.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/accumulo/core/logging/ConditionalLogger.java
Outdated
Show resolved
Hide resolved
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. |
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/accumulo/core/logging/ConditionalLogger.java
Show resolved
Hide resolved
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/accumulo/core/logging/ConditionalLogger.java
Outdated
Show resolved
Hide resolved
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
Outdated
Show resolved
Hide resolved
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
Outdated
Show resolved
Hide resolved
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
Related to #4539