Repository navigation
2.1 backport logrecovery - #6395
amcdonaldccri wants to merge 4 commits into
Conversation
909d002 to
8333a39
Compare
|
I ran all the ITs on this branch. There are no tests failing on this branch that are not also failing on the 2.1 branch. |
| return null; | ||
| } | ||
|
|
||
| public static LogEntry switchVolume(LogEntry le, List<Pair<Path,Path>> replacements) { |
There was a problem hiding this comment.
I think the better solution would be to just make the method below public.
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import com.google.common.cache.Cache; |
There was a problem hiding this comment.
I think this should be com.github.benmanes.caffeine.cache.Cache in all new imports
There was a problem hiding this comment.
Ok, I broke it up into two commits one for RecoveryLogsIterator, which touched 15 files, and the other for the rest of the files using com.google.common.cache.Cache (9 src, 1 test and the manager pom.xml)
There was a problem hiding this comment.
I've squashed the last 3 commits.
There was a problem hiding this comment.
If there is anything else that needs to be done, please let me know. This is my first time contributing to any opensource project and I apologize in advance.
There was a problem hiding this comment.
Discussed this with @dlmarion and he was only suggesting to change the new import in this file. Since 2.1 is in bugfix mode where we are trying to keep it as stable as possible, it would probably be good to keep all the existing 2.1 code that is currently using a guava cache as-is.
9330562 to
95a2651
Compare
|
The build is failing because some of our formatting checks are failing. If you run |
95a2651 to
1384261
Compare
|
Thanks! Obviously didn't see in GitHub what the issue was. And the import alphabetize plugin is a pretty slick feature. |
1384261 to
7d7aa9d
Compare
7d7aa9d to
26b1bd2
Compare
|
I did some measurements comparing rel/2.1.6 and 2.1.6 with this change. |
|
@amcdonaldccri - I just wanted to let you know that I'm monitoring this issue. I see you have kept it up to date, and I appreciate that. It hasn't been merged because the person best suited to look at it hasn't looked at it yet. Hopefully that will change soon. |
|
@dlmarion |
This comment was marked as spam.
This comment was marked as spam.
…ata apache#4873 This commit makes two major changes. First it changed log recovery to use block caches. Second it checks if a tablet has any data in walogs before acquiring the recovery lock. These two changes together really speed up loading tablets that have no data in walogs. These changes introduce an extra opening of the walogs to see if the recovery lock needs to be acquired. Using the block caches for this extra opening should avoid any extra cost. The block caches also help in the case where many tablets with the same walogs are assigned to a tablet server. In some simple test saw an 8x speedup in tablet load times. Anytime a tablet has an unclean shutdown it will have the walogs of the dead tserver assigned to it even if had no data in those walogs. These change make loading tablets in that situation much faster. {"fundingSource": "41603", "team": "FED.ICGSA.OPS.MOE", "fshGit": "e6b31a7b", "fshDocker": "sha256:c0f3a474"}
In apache#4873 a check was added to inspect walogs during tablet load to see if they had any data for the tablet. This check happens prior to volume replacement that also runs during tablet load. Therefore if volume replacement is needed for the walogs then this check will fail because it can not find the files and the tablet will fail to load. To fix this problem modified the new check to switch volumes if needed prior to running the check. {"fundingSource": "41201", "team": "FED.ICGSA.OPS.MOE", "fshGit": "dummy-lo", "fshDocker": "sha256:20cf0045"}
… log recovery. (apache#4874) The log recovery code would list the sorted walog files multiple times during recovery. These changes modify the code to only list the files once. Also the listing is cached for a short period of time to improve the case of multiple tablet referencing the same walogs. This along with apache#4873 should result in much less traffic to the namenode when an entire accumulo cluster shutsdown and needs to recover. {"fundingSource": "41203", "team": "FED.ICGSA.OPS.MOE", "fshGit": "e6b31a7b", "fshDocker": "sha256:c0f3a474"}
26b1bd2 to
663fe80
Compare
|
@amcdonaldccri Looks like you just need some formatting changes for |
added var unused = in Combiner sawDelete and CompactionHelper
663fe80 to
6d6edd6
Compare
|
yep, not used to having mvn change my code :/ ... I was just happy that |
keith-turner
left a comment
keith-turner
left a comment
There was a problem hiding this comment.
Still looking through these changes, posting what I have found so far.
| * diagnostic tools that operate directly on sorted recovery logs without going through the normal | ||
| * recovery flow with LogEntry objects. | ||
| */ | ||
| public static ResolvedSortedLog fromSortedLogDir(Path sortedLogDir, VolumeManager fs) |
There was a problem hiding this comment.
This method seems to mostly duplicate the code in the resolve method. What is preventing use of the resolve method?
| LOG.debug("Opening recovery log dir {}", logDir.getName()); | ||
| for (ResolvedSortedLog logDir : recoveryLogDirs) { | ||
| LOG.debug("Opening recovery log dir {}", logDir.getDir().getName()); | ||
| TreeMap<Path,FileStatus> logFiles = getFiles(vm, logDir); |
There was a problem hiding this comment.
I think we need to make the following change and remove the getFiles() method. The getFiles method is accessing DFS to do a file listing which is what this change is tying to avoid. The old code used to return the FileStatus because to open the file we need to know its length to seek to the end. It used to use the FileStatus to get the length. However, these new changes are now using the file length cache to get the file length, so the FileStatus is hopefully no longer needed with these changes.
| TreeMap<Path,FileStatus> logFiles = getFiles(vm, logDir); | |
| SortedSet<Path> logFiles = logDir.getChildren(); |
This branch has not been deployed
6 participants
closes #4887
Saw this issue #4887 and thought I could help.
And using AI's help I got it to pass the unit tests.
These are the Integration Tests that got stuck or timed out.
org.apache.accumulo.test.fate.zookeeper.FateIT never returned, but ran it again and it passed
org.apache.accumulo.test.tracing.ScanTracingIT timed out twice
org.apache.accumulo.test.functional.MetadataMaxFilesIT timed out twice
org.apache.accumulo.test.functional.TimeoutIT timed out twice
org.apache.accumulo.test.functional.TServerShutdownOptimizationsIT timed out twice
org.apache.accumulo.test.functional.KerberosIT timed out twice
org.apache.accumulo.test.shell.ShellServerIT never returned, but ran it again and it passed twice
[ERROR] Tests run: 11, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 166.9 s <<< FAILURE! -- in org.apache.accumulo.test.functional.KerberosIT
[ERROR] org.apache.accumulo.test.functional.KerberosIT.testGetDelegationTokenDenied -- Time elapsed: 14.48 s <<< ERROR!
java.lang.IllegalStateException: org.apache.hadoop.security.KerberosAuthException: failure to login: using ticket cache file: FILE:/tmp/krb5cc_911602271_DwR0dF javax.security.auth.login.LoginException: java.lang
.IllegalArgumentException: Illegal principal name ajmcdonald@CCRI.COM: org.apache.hadoop.security.authentication.util.KerberosName$NoMatchingRule: No rules applied to ajmcdonald@CCRI.COM
I would like to deploy it to one of our dev environments and do some ingest testing but haven't gotten to it yet.