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.

Sign up for GitHub

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

Jump to bottom

Handle old sorted map files in Upgrade #2185

Merged
merged 3 commits into from
Jul 30, 2021
Merged

Handle old sorted map files in Upgrade #2185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
329f1c6
Add upgradeFiles to Upgrade code
milleruntime Jun 29, 2021
9eb10dc
Fix typo
milleruntime Jul 2, 2021
03e72b3
Revert new upgradeFiles and put method call in metadata upgrade
milleruntime Jul 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 41 additions & 0 deletions server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader9to10.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,12 @@
* <a href="https://github.com/apache/accumulo/issues/1642">#1642</a>, and
* <a href="https://github.com/apache/accumulo/issues/1643">#1643</a> as well.</li>
* </ul>
*
* Sorted recovery was updated to use RFiles instead of map files. So to prevent issues during
* tablet recovery, remove the old temporary map files and resort using RFiles. This is done in
* {@link #dropSortedMapWALFiles(VolumeManager)}. For more information see the following issues:
* <a href="https://github.com/apache/accumulo/issues/2117">#2117</a> and
* <a href="https://github.com/apache/accumulo/issues/2179">#2179</a>
*/
public class Upgrader9to10 implements Upgrader {

Expand Down Expand Up @@ -142,6 +148,8 @@ public void upgradeMetadata(ServerContext ctx) {
upgradeRelativePaths(ctx, Ample.DataLevel.USER);
upgradeDirColumns(ctx, Ample.DataLevel.USER);
upgradeFileDeletes(ctx, Ample.DataLevel.USER);
// special case where old files need to be deleted
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider calling this new method in upgradeZookeeper instead? I think that is called before the root tablet is loaded, which would allow deleting any old sorted logs the root tablet may reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I didn't think any tables were loaded until after the Upgrader was finished. If that is the case, then moving it to upgradeZookeeper would be better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the upgrade methods are run as follows.

  • upgradeZookeeper() is run before the root tablet is loaded.
  • upgradeRoot() is run after the root tablet is loaded and before the metadata table is loaded.
  • upgradeMetadata is run after the metadata table is loaded and before loading user tablets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah OK. I will move it to upgradeZookeeper().

dropSortedMapWALFiles(ctx.getVolumeManager());
milleruntime marked this conversation as resolved.
Show resolved Hide resolved
}

private void setMetaTableProps(ServerContext ctx) {
Expand Down Expand Up @@ -726,4 +734,37 @@ static Path resolveRelativeDelete(String oldDelete, String upgradeProperty) {
}
return new Path(upgradeProperty, VolumeManager.FileType.TABLE.getDirectory() + oldDelete);
}

/**
* Remove old temporary map files to prevent problems during recovery.
*/
static void dropSortedMapWALFiles(VolumeManager vm) {
Path recoveryDir = new Path("/accumulo/recovery");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this should loop over the set of configured volumes instead of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I will create a follow on ticket with this and any other suggestions you have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think using the volumes configured in instance.volumes will be enough?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think using the volumes configured in instance.volumes will be enough?

Yeah, could look through all of those. Could narrow it by calling :

accumulo/server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManager.java

Line 180 in a9100ac

Set<String> choosable(org.apache.accumulo.core.spi.fs.VolumeChooserEnvironment env,

But I don't think it hurts to just look through all volumes. Maybe could use

accumulo/server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManager.java

Line 196 in a9100ac

Collection<Volume> getVolumes();
to get the list of volumes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would probably be best to avoid choosable() as that will give the volumes configured for new WALs. The config could change and an old WAL could be on a volume that choosable() no longer returns. So probably best to inspect all volumes looking for old sorted wals to nuke.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would probably be best to avoid choosable() as that will give the volumes configured for new WALs. The config could change and an old WAL could be on a volume that choosable() no longer returns. So probably best to inspect all volumes looking for old sorted wals to nuke.

Definitely avoid this. Consider RandomVolumeChooser 😺

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the VolumeChooser was for selecting volumes for writes? I want to loop through every volume configured to find the old sorted WALs to remove. Will the VolumeChooser allow for this?

try {
if (!vm.exists(recoveryDir)) {
log.info("There are no recovery files in /accumulo/recovery");
return;
}
List<Path> directoriesToDrop = new ArrayList<>();
for (FileStatus walDir : vm.listStatus(recoveryDir)) {
// map files will be in a directory starting with "part"
Path walDirPath = walDir.getPath();
for (FileStatus dirOrFile : vm.listStatus(walDirPath)) {
if (dirOrFile.isDirectory()) {
directoriesToDrop.add(walDirPath);
break;
}
}
}
if (!directoriesToDrop.isEmpty()) {
log.info("Found {} old sorted map directories to delete.", directoriesToDrop.size());
for (Path dir : directoriesToDrop) {
log.info("Deleting everything in old sorted map directory: {}", dir);
vm.deleteRecursively(dir);
}
}
} catch (IOException ioe) {
throw new UncheckedIOException(ioe);
}
}
}
42 changes: 42 additions & 0 deletions server/manager/src/test/java/org/apache/accumulo/manager/upgrade/Upgrader9to10Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@
import static org.easymock.EasyMock.expect;
import static org.easymock.EasyMock.expectLastCall;
import static org.easymock.EasyMock.replay;
import static org.easymock.EasyMock.reset;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
Expand All @@ -49,6 +51,7 @@
import org.apache.accumulo.core.security.Authorizations;
import org.apache.accumulo.server.fs.VolumeManager;
import org.apache.accumulo.server.gc.GcVolumeUtil;
import org.apache.hadoop.fs.FileStatus;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.io.Text;
import org.junit.Test;
Expand Down Expand Up @@ -324,4 +327,43 @@ private void verifyPathsReplaced(List<Mutation> expected, List<Mutation> results

assertEquals("Replacements should have update for every delete", deleteCount, updateCount);
}

@Test
public void testDropSortedMapWALs() throws IOException {
Path recoveryDir = new Path("/accumulo/recovery");
VolumeManager fs = createMock(VolumeManager.class);
FileStatus[] dirs = new FileStatus[2];
dirs[0] = createMock(FileStatus.class);
Path dir0 = new Path("/accumulo/recovery/A123456789");
FileStatus[] dir0Files = new FileStatus[1];
dir0Files[0] = createMock(FileStatus.class);
dirs[1] = createMock(FileStatus.class);
Path dir1 = new Path("/accumulo/recovery/B123456789");
FileStatus[] dir1Files = new FileStatus[1];
dir1Files[0] = createMock(FileStatus.class);
Path part1Dir = new Path("/accumulo/recovery/B123456789/part-r-0000");

expect(fs.exists(recoveryDir)).andReturn(true).once();
expect(fs.listStatus(recoveryDir)).andReturn(dirs).once();
expect(dirs[0].getPath()).andReturn(dir0).once();
expect(fs.listStatus(dir0)).andReturn(dir0Files).once();
expect(dir0Files[0].isDirectory()).andReturn(false).once();

expect(dirs[1].getPath()).andReturn(dir1).once();
expect(fs.listStatus(dir1)).andReturn(dir1Files).once();
expect(dir1Files[0].isDirectory()).andReturn(true).once();
expect(dir1Files[0].getPath()).andReturn(part1Dir).once();

expect(fs.deleteRecursively(dir1)).andReturn(true).once();

replay(fs, dirs[0], dirs[1], dir0Files[0], dir1Files[0]);
Upgrader9to10.dropSortedMapWALFiles(fs);

reset(fs);

// test case where there is no recovery
expect(fs.exists(recoveryDir)).andReturn(false).once();
replay(fs);
Upgrader9to10.dropSortedMapWALFiles(fs);
}
}