Open
Conversation
This commit addresses things that are problems in newer Java versions, but attempts to get ahead of the issues, to ease the migration to newer versions. The logging changes are done here at low risk for Accumulo 2.1, since the default logging configuration can be easily changed by users, and the only affected log messages are trace level and not typically seen by most users. * Remove logging uses of `Thread.getId()` in code (deprecated in 21) and replace with logging config that shows the thread information * Additional minor changes to `log4j2-test.properties` files to make it easier to compare them and keep them in sync * Add Threads.toString(Thread) method for one case where logging more information about the thread than just the Thread ID would be useful (in TabletServerBatchWriter.java) * Replace use of `new URL(String)` (deprecated in 21) with `URI.create(String).toURL()` * Remove unneeded MiniDFSUtil that was using a deprecated exec method; this code is not needed because Hadoop already sets a suitable default value for the `dfs.datanode.data.dir.perm` and we don't need to compute a different one using this code; similarly remove the unnecessary `hadoop.security.token.service.use_ip` property which was just setting the default value. * Remove unused `CollectTabletStats` test-only class and its corresponding test case, since it is not used by any test code (this removes another case where the deprecated `Thread.getId()` was being called.
keith-turner
approved these changes
Mar 5, 2026
Contributor
keith-turner
left a comment
keith-turner
left a comment
There was a problem hiding this comment.
Everything looks good, bit worried about the thread names in each log message though. Have not looked at it what it looks like though.
| appender.console.target = SYSTEM_ERR | ||
| appender.console.layout.type = PatternLayout | ||
| appender.console.layout.pattern = %d{ISO8601} [%-8c{2}] %-5p: %m%n | ||
| appender.console.layout.pattern = %d{ISO8601} [%T;%t] [%-8c{2}] %-5p: %m%n |
Contributor
There was a problem hiding this comment.
Some code sets thread names to make jstacks more informative like here. Including the thread name on each log message could be a lot for those cases.
Have you run ITs w/ these configs? Wonder what that looks like.
| return instanceId; | ||
| } | ||
|
|
||
| @Deprecated |
Contributor
There was a problem hiding this comment.
Why did this annotation need to be added?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit addresses things that are problems in newer Java versions, but attempts to get ahead of the issues, to ease the migration to newer versions. The logging changes are done here at low risk for Accumulo 2.1, since the default logging configuration can be easily changed by users, and the only affected log messages are trace level and not typically seen by most users.
Thread.getId()in code (deprecated in 21) and replace with logging config that shows the thread informationlog4j2-test.propertiesfiles to make it easier to compare them and keep them in syncnew URL(String)(deprecated in 21) withURI.create(String).toURL()dfs.datanode.data.dir.permand we don't need to compute a different one using this code; similarly remove the unnecessaryhadoop.security.token.service.use_ipproperty which was just setting the default value.CollectTabletStatstest-only class and its corresponding test case, since it is not used by any test code (this removes another case where the deprecatedThread.getId()was being called.