You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@accumulo.apache.org by kt...@apache.org on 2014/03/24 18:09:23 UTC
git commit: ACCUMULO-2520 made GC validate data read from !METADATA
table
Repository: accumulo
Updated Branches:
refs/heads/1.4.5-SNAPSHOT 4fdefd783 -> ca3f77894
ACCUMULO-2520 made GC validate data read from !METADATA table
Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo
Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/ca3f7789
Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/ca3f7789
Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/ca3f7789
Branch: refs/heads/1.4.5-SNAPSHOT
Commit: ca3f77894466caddf71c89d78a98f9fee8fc7fd8
Parents: 4fdefd7
Author: Keith Turner <kt...@apache.org>
Authored: Fri Mar 21 14:58:05 2014 -0400
Committer: Keith Turner <kt...@apache.org>
Committed: Mon Mar 24 10:54:00 2014 -0400
----------------------------------------------------------------------
.../server/gc/SimpleGarbageCollector.java | 48 ++++++++++++++++
.../server/test/GCLotsOfCandidatesTest.java | 2 +-
.../server/gc/SimpleGarbageCollectorTest.java | 58 ++++++++++++++++++++
.../accumulo/server/gc/TestConfirmDeletes.java | 35 ++++++++++++
test/system/auto/simple/gc.py | 9 +++
5 files changed, 151 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/accumulo/blob/ca3f7789/src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java
----------------------------------------------------------------------
diff --git a/src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java b/src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java
index 22c3c0e..d09c557 100644
--- a/src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java
+++ b/src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java
@@ -31,6 +31,7 @@ import java.util.TreeSet;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
+import java.util.regex.Pattern;
import org.apache.accumulo.cloudtrace.instrument.CountSampler;
import org.apache.accumulo.cloudtrace.instrument.Sampler;
@@ -108,6 +109,9 @@ public class SimpleGarbageCollector implements Iface {
private static final Logger log = Logger.getLogger(SimpleGarbageCollector.class);
+ private static final Pattern dirPattern = Pattern.compile("/[\\p{Punct}\\p{Alnum}&&[^/]]+/[\\p{Punct}\\p{Alnum}&&[^/]]+");
+ private static final Pattern filePattern = Pattern.compile("/[\\p{Punct}\\p{Alnum}&&[^/]]+/[\\p{Punct}\\p{Alnum}&&[^/]]+/[\\p{Punct}\\p{Alnum}&&[^/]]+");
+
private Instance instance;
private AuthInfo credentials;
private long gcStartDelay;
@@ -419,6 +423,43 @@ public class SimpleGarbageCollector implements Iface {
return new InetSocketAddress(Accumulo.getLocalAddress(new String[] {"--address", address}), port);
}
+ // visible for testing
+ static boolean isValidFileDelete(String path) {
+ return filePattern.matcher(path).matches();
+ }
+
+ // visible for testing
+ static boolean isValidDirDelete(String path) {
+ return dirPattern.matcher(path).matches();
+ }
+
+ // visible for testing
+ static boolean isValidCandidate(String candidate) {
+ return isValidDirDelete(candidate) || isValidFileDelete(candidate);
+ }
+
+ private void validateDir(String path) {
+ if (!isValidDirDelete(path)) {
+ throw new IllegalArgumentException("Invalid dir " + path);
+ }
+ }
+
+ private void validateFile(String delete) {
+ if (!isValidFileDelete(delete)) {
+ throw new IllegalArgumentException("Invalid file " + delete);
+ }
+ }
+
+ private void removeInvalidCandidates(SortedSet<String> candidates) {
+ for (Iterator<String> iterator = candidates.iterator(); iterator.hasNext();) {
+ String candidate = (String) iterator.next();
+ if (!isValidCandidate(candidate)) {
+ log.error("Ingoring invalid deletion candidate " + candidate);
+ iterator.remove();
+ }
+ }
+ }
+
/**
* This method gets a set of candidates for deletion by scanning the METADATA table deleted flag keyspace
*/
@@ -441,6 +482,8 @@ public class SimpleGarbageCollector implements Iface {
log.error("Unable to check the filesystem for offline candidates. Removing all candidates for deletion to be safe.", e);
candidates.clear();
}
+
+ removeInvalidCandidates(candidates);
return candidates;
}
@@ -469,6 +512,7 @@ public class SimpleGarbageCollector implements Iface {
}
}
+ removeInvalidCandidates(candidates);
return candidates;
}
@@ -513,6 +557,7 @@ public class SimpleGarbageCollector implements Iface {
for (Entry<Key,Value> entry : scanner) {
String blipPath = entry.getKey().getRow().toString().substring(Constants.METADATA_BLIP_FLAG_PREFIX.length());
+ validateDir(blipPath);
Iterator<String> tailIter = candidates.tailSet(blipPath).iterator();
int count = 0;
while (tailIter.hasNext()) {
@@ -556,15 +601,18 @@ public class SimpleGarbageCollector implements Iface {
}
// WARNING: This line is EXTREMELY IMPORTANT.
// You MUST REMOVE candidates that are still in use
+ validateFile(delete);
if (candidates.remove(delete))
log.debug("Candidate was still in use in the METADATA table: " + delete);
String path = delete.substring(0, delete.lastIndexOf('/'));
+ validateDir(path);
if (candidates.remove(path))
log.debug("Candidate was still in use in the METADATA table: " + path);
} else if (Constants.METADATA_DIRECTORY_COLUMN.hasColumns(entry.getKey())) {
String table = new String(KeyExtent.tableOfMetadataRow(entry.getKey().getRow()));
String delete = "/" + table + entry.getValue().toString();
+ validateDir(delete);
if (candidates.remove(delete))
log.debug("Candidate was still in use in the METADATA table: " + delete);
} else
http://git-wip-us.apache.org/repos/asf/accumulo/blob/ca3f7789/src/server/src/main/java/org/apache/accumulo/server/test/GCLotsOfCandidatesTest.java
----------------------------------------------------------------------
diff --git a/src/server/src/main/java/org/apache/accumulo/server/test/GCLotsOfCandidatesTest.java b/src/server/src/main/java/org/apache/accumulo/server/test/GCLotsOfCandidatesTest.java
index 722fd1a..c984a94 100644
--- a/src/server/src/main/java/org/apache/accumulo/server/test/GCLotsOfCandidatesTest.java
+++ b/src/server/src/main/java/org/apache/accumulo/server/test/GCLotsOfCandidatesTest.java
@@ -45,7 +45,7 @@ public class GCLotsOfCandidatesTest {
for (int i = 0; i < 10000; ++i) {
final Text emptyText = new Text("");
- Text row = new Text(String.format("%s%s%020d%s", Constants.METADATA_DELETE_FLAG_PREFIX, "/", i,
+ Text row = new Text(String.format("%s/%020d/%s", Constants.METADATA_DELETE_FLAG_PREFIX, i,
"aaaaaaaaaabbbbbbbbbbccccccccccddddddddddeeeeeeeeeeffffffffffgggggggggghhhhhhhhhhiiiiiiiiiijjjjjjjjjj"));
Mutation delFlag = new Mutation(row);
delFlag.put(emptyText, emptyText, new Value(new byte[] {}));
http://git-wip-us.apache.org/repos/asf/accumulo/blob/ca3f7789/src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java
----------------------------------------------------------------------
diff --git a/src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java b/src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java
new file mode 100644
index 0000000..c89aad3
--- /dev/null
+++ b/src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.server.gc;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class SimpleGarbageCollectorTest {
+ @Test
+ public void testValidation() {
+
+ Assert.assertTrue(SimpleGarbageCollector.isValidDirDelete("/a/b"));
+ Assert.assertTrue(SimpleGarbageCollector.isValidDirDelete("/45/t-00004"));
+ Assert.assertTrue(SimpleGarbageCollector.isValidDirDelete("/45/b-00006"));
+
+ Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete(""));
+ Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("/"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("/a"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("//a"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("//"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("////"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("/45//t-00004"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("45///b-00006"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("/45/b-00006/C00000.rf"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("/ /b"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("/a/b/c"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("zz/a/b"));
+
+ Assert.assertTrue(SimpleGarbageCollector.isValidFileDelete("/45/t-000c4/C00000.rf"));
+ Assert.assertTrue(SimpleGarbageCollector.isValidFileDelete("/a7/b-00006/F0000a.rf"));
+
+ Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete(""));
+ Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("/"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("a7/b-00006/F0000a.rf"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("//a7/b-00006/F0000a.rf"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("///"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("//////"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("///F0000a.rf"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("hh/a7/b-00006/F0000a.rf"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("/a7/b-00006/"));
+ Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("/a7/b-00006"));
+
+ }
+}
http://git-wip-us.apache.org/repos/asf/accumulo/blob/ca3f7789/src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java
----------------------------------------------------------------------
diff --git a/src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java b/src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java
index be444dd..16a1912 100644
--- a/src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java
+++ b/src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java
@@ -53,6 +53,37 @@ public class TestConfirmDeletes {
return result;
}
+ @Test(expected = IllegalArgumentException.class)
+ public void testInvalidBlip() throws Exception {
+ String metadata[] = new String[] {"1636< file:/b-0001/someFile 10,100", "1636< last:3353986642a66eb 192.168.117.9:9997",
+ "1636< srv:dir /default_tablet", "1636< srv:flush 2", "1636< srv:lock tservers/192.168.117.9:9997/zlock-0000000000$3353986642a66eb",
+ "1636< srv:time M1328505870023", "1636< ~tab:~pr \0"};
+ String deletes[] = {"~blip/", "~del/1636/b-0001/someFile"};
+
+ test1(metadata, deletes, 1, 0);
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testInvalidFile() throws Exception {
+ String metadata[] = new String[] {"1636< file:/b-0001/someFile/ooopppsss/garbage 10,100", "1636< last:3353986642a66eb 192.168.117.9:9997",
+ "1636< srv:dir /default_tablet", "1636< srv:flush 2", "1636< srv:lock tservers/192.168.117.9:9997/zlock-0000000000$3353986642a66eb",
+ "1636< srv:time M1328505870023", "1636< ~tab:~pr \0"};
+ String deletes[] = {"~del/1636/b-0001/someFile"};
+
+ test1(metadata, deletes, 1, 0);
+ }
+
+ @Test
+ public void testInvalidDeletes() throws Exception {
+ // a few invalid deletes, should be ignored
+ String metadata[] = new String[] {"1636< file:/default_tablet/someFile 10,100", "1636< last:3353986642a66eb 192.168.117.9:9997",
+ "1636< srv:dir /default_tablet", "1636< srv:flush 2", "1636< srv:lock tservers/192.168.117.9:9997/zlock-0000000000$3353986642a66eb",
+ "1636< srv:time M1328505870023", "1636< ~tab:~pr \0"};
+ String deletes[] = {"~del", "~del/"};
+
+ test1(metadata, deletes, 0, 0);
+ }
+
@Test
public void test() throws Exception {
@@ -88,6 +119,10 @@ public class TestConfirmDeletes {
deletes = new String[] {"~del/9/default_tablet", "~del/9/default_tablet/someFile"};
test1(metadata, deletes, 2, 0);
+ metadata = new String[] {"1636< file:/b-0001/I0000 10,100", "1636< last:3353986642a66eb 192.168.117.9:9997", "1636< srv:dir /default_tablet",
+ "1636< srv:flush 2", "1636< srv:lock tservers/192.168.117.9:9997/zlock-0000000000$3353986642a66eb", "1636< srv:time M1328505870023",
+ "1636< ~tab:~pr \0"};
+
deletes = new String[] {"~blip/1636/b-0001", "~del/1636/b-0001/I0000"};
test1(metadata, deletes, 1, 0);
}
http://git-wip-us.apache.org/repos/asf/accumulo/blob/ca3f7789/test/system/auto/simple/gc.py
----------------------------------------------------------------------
diff --git a/test/system/auto/simple/gc.py b/test/system/auto/simple/gc.py
index 4697246..2b7e2c4 100755
--- a/test/system/auto/simple/gc.py
+++ b/test/system/auto/simple/gc.py
@@ -60,6 +60,8 @@ class GCTest(SunnyDayTest):
def runTest(self):
self.waitForStop(self.ingester, 60)
self.shell(self.masterHost(), 'flush -t test_ingest')
+ #following statemets caused GC to delete everything ACCUMULO-2520
+ self.shell(self.masterHost(), 'grant Table.WRITE -u root -t !METADATA\ntable !METADATA\ninsert ~del testDel test valueTest\n')
self.stop_gc(self.masterHost())
count = self.waitForFileCountToStabilize()
@@ -74,6 +76,13 @@ class GCTest(SunnyDayTest):
glob.glob(os.path.join(ACCUMULO_HOME,'logs',ID,'gc_*')))
out, err = handle.communicate()
self.assert_(handle.returncode != 0)
+
+ handle = self.runOn(self.masterHost(),
+ ['grep', '-q', 'Ingoring invalid deletion candidate'] +
+ glob.glob(os.path.join(ACCUMULO_HOME,'logs',ID,'gc_*')))
+ out, err = handle.communicate()
+ self.assert_(handle.returncode == 0)
+
self.pkill(self.masterHost(), 'java.*Main gc$', signal.SIGHUP)
self.wait(gc)
log.info("Verifying Ingestion")