Skip to content
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

Merged
merged 8 commits into from
May 24, 2024

Conversation

dlmarion
Copy link
Contributor

Related to #4539

dlmarion added 3 commits May 14, 2024 16:45
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.
Copy link
Contributor

@cshannon cshannon left a 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.

@dlmarion
Copy link
Contributor Author

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.

@cshannon
Copy link
Contributor

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.

@dlmarion dlmarion merged commit 221259e into apache:2.1 May 24, 2024
8 checks passed
@dlmarion dlmarion deleted the 4539-log-long-tablet-close branch May 24, 2024 16:46
@ctubbsii ctubbsii modified the milestones: 3.1.0, 2.1.3 Jul 12, 2024
@ctubbsii
Copy link
Member

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.

ctubbsii added a commit to ctubbsii/accumulo that referenced this pull request Jul 16, 2024
* 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
ctubbsii added a commit to ctubbsii/accumulo that referenced this pull request Jul 17, 2024
* 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)
ctubbsii added a commit that referenced this pull request Jul 18, 2024
* 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)
ctubbsii added a commit to ctubbsii/accumulo that referenced this pull request Jul 22, 2024
* 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
@ctubbsii ctubbsii mentioned this pull request Jul 22, 2024
ctubbsii added a commit that referenced this pull request Jul 24, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants