shaofengshi closed pull request #301: KYLIN-3633 Avoid potential dead lock when building global dictionary URL: https://github.com/apache/kylin/pull/301
This is a PR merged from a forked repository. As GitHub hides the original diff on merge, it is displayed below for the sake of provenance: As this is a foreign pull request (from a fork), the diff is supplied below (as it won't show otherwise due to GitHub magic): diff --git a/core-dictionary/src/main/java/org/apache/kylin/dict/DictionaryGenerator.java b/core-dictionary/src/main/java/org/apache/kylin/dict/DictionaryGenerator.java index db0c302970..7c33b4a1e4 100644 --- a/core-dictionary/src/main/java/org/apache/kylin/dict/DictionaryGenerator.java +++ b/core-dictionary/src/main/java/org/apache/kylin/dict/DictionaryGenerator.java @@ -75,13 +75,19 @@ public static IDictionaryBuilder newDictionaryBuilder(DataType dataType) { builder.init(dictInfo, baseId, null); // add values - while (valueEnumerator.moveNext()) { - String value = valueEnumerator.current(); + try { + while (valueEnumerator.moveNext()) { + String value = valueEnumerator.current(); - boolean accept = builder.addValue(value); + boolean accept = builder.addValue(value); - if (accept && samples.size() < nSamples && samples.contains(value) == false) - samples.add(value); + if (accept && samples.size() < nSamples && samples.contains(value) == false) + samples.add(value); + } + } catch (IOException e) { + logger.error("Error during adding dict value.", e); + builder.clear(); + throw e; } // build @@ -149,6 +155,12 @@ public boolean addValue(String value) { return new DateStrDictionary(datePattern, baseId); } + + + @Override + public void clear() { + // do nothing + } } private static class TimeDictBuilder implements IDictionaryBuilder { @@ -171,6 +183,11 @@ public boolean addValue(String value) { public Dictionary<String> build() throws IOException { return new TimeStrDictionary(); // base ID is always 0 } + + @Override + public void clear() { + + } } private static class StringTrieDictBuilder implements IDictionaryBuilder { @@ -196,6 +213,11 @@ public boolean addValue(String value) { public Dictionary<String> build() throws IOException { return builder.build(baseId); } + + @Override + public void clear() { + + } } private static class StringTrieDictForestBuilder implements IDictionaryBuilder { @@ -219,6 +241,11 @@ public boolean addValue(String value) { public Dictionary<String> build() throws IOException { return builder.build(); } + + @Override + public void clear() { + + } } @SuppressWarnings("deprecation") @@ -245,6 +272,11 @@ public boolean addValue(String value) { public Dictionary<String> build() throws IOException { return builder.build(baseId); } + + @Override + public void clear() { + + } } private static class NumberTrieDictForestBuilder implements IDictionaryBuilder { @@ -268,6 +300,11 @@ public boolean addValue(String value) { public Dictionary<String> build() throws IOException { return builder.build(); } + + @Override + public void clear() { + + } } } diff --git a/core-dictionary/src/main/java/org/apache/kylin/dict/GlobalDictionaryBuilder.java b/core-dictionary/src/main/java/org/apache/kylin/dict/GlobalDictionaryBuilder.java index 9168ca4a1a..d813793ea1 100644 --- a/core-dictionary/src/main/java/org/apache/kylin/dict/GlobalDictionaryBuilder.java +++ b/core-dictionary/src/main/java/org/apache/kylin/dict/GlobalDictionaryBuilder.java @@ -19,8 +19,8 @@ package org.apache.kylin.dict; import java.io.IOException; - import java.util.Locale; + import org.apache.kylin.common.KylinConfig; import org.apache.kylin.common.lock.DistributedLock; import org.apache.kylin.common.util.Dictionary; @@ -104,6 +104,13 @@ public boolean addValue(String value) { return new AppendTrieDictionary<>(); } + @Override + public void clear() { + if (lock.isLocked(getLockPath(sourceColumn))) { + lock.unlock(getLockPath(sourceColumn)); + } + } + private String getLockPath(String pathName) { return "/dict/" + pathName + "/lock"; } diff --git a/core-dictionary/src/main/java/org/apache/kylin/dict/IDictionaryBuilder.java b/core-dictionary/src/main/java/org/apache/kylin/dict/IDictionaryBuilder.java index e2a643dbd0..771bfb42c6 100644 --- a/core-dictionary/src/main/java/org/apache/kylin/dict/IDictionaryBuilder.java +++ b/core-dictionary/src/main/java/org/apache/kylin/dict/IDictionaryBuilder.java @@ -35,4 +35,7 @@ /** Build the dictionary */ Dictionary<String> build() throws IOException; + + /** Clear before exit */ + void clear(); } diff --git a/core-dictionary/src/main/java/org/apache/kylin/dict/global/SegmentAppendTrieDictBuilder.java b/core-dictionary/src/main/java/org/apache/kylin/dict/global/SegmentAppendTrieDictBuilder.java index f8640a0b2a..770b0bc193 100644 --- a/core-dictionary/src/main/java/org/apache/kylin/dict/global/SegmentAppendTrieDictBuilder.java +++ b/core-dictionary/src/main/java/org/apache/kylin/dict/global/SegmentAppendTrieDictBuilder.java @@ -78,4 +78,9 @@ public boolean addValue(String value) { public Dictionary<String> build() throws IOException { return builder.build(baseId); } + + @Override + public void clear() { + + } } diff --git a/kylin-it/src/test/java/org/apache/kylin/dict/ITGlobalDictionaryBuilderTest.java b/kylin-it/src/test/java/org/apache/kylin/dict/ITGlobalDictionaryBuilderTest.java index c578a57c34..94c4f56640 100644 --- a/kylin-it/src/test/java/org/apache/kylin/dict/ITGlobalDictionaryBuilderTest.java +++ b/kylin-it/src/test/java/org/apache/kylin/dict/ITGlobalDictionaryBuilderTest.java @@ -18,20 +18,21 @@ package org.apache.kylin.dict; +import java.io.IOException; +import java.util.concurrent.CountDownLatch; + import org.apache.hadoop.fs.Path; import org.apache.kylin.common.KylinConfig; +import org.apache.kylin.common.lock.DistributedLock; import org.apache.kylin.common.util.Dictionary; import org.apache.kylin.common.util.HBaseMetadataTestCase; import org.apache.kylin.common.util.HadoopUtil; import org.junit.After; +import org.junit.Assert; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; - -import java.io.IOException; -import java.util.concurrent.CountDownLatch; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotEquals; +import org.junit.rules.ExpectedException; public class ITGlobalDictionaryBuilderTest extends HBaseMetadataTestCase { private DictionaryInfo dictionaryInfo; @@ -48,8 +49,12 @@ public void afterTest() { staticCleanupTestMetadata(); } + @Rule + public ExpectedException thrown = ExpectedException.none(); + private void cleanup() { - String BASE_DIR = KylinConfig.getInstanceFromEnv().getHdfsWorkingDirectory() + "/resources/GlobalDict" + dictionaryInfo.getResourceDir() + "/"; + String BASE_DIR = KylinConfig.getInstanceFromEnv().getHdfsWorkingDirectory() + "/resources/GlobalDict" + + dictionaryInfo.getResourceDir() + "/"; Path basePath = new Path(BASE_DIR); try { HadoopUtil.getFileSystem(basePath).delete(basePath, true); @@ -77,16 +82,33 @@ public void testGlobalDictLock() throws IOException, InterruptedException { Dictionary<String> dict = builder.build(); for (int i = 0; i < 10000; i++) { - assertNotEquals(-1, dict.getIdFromValue("t1_" + i)); + Assert.assertNotEquals(-1, dict.getIdFromValue("t1_" + i)); } for (int i = 0; i < 10; i++) { - assertNotEquals(-1, dict.getIdFromValue("t2_" + i)); + Assert.assertNotEquals(-1, dict.getIdFromValue("t2_" + i)); } for (int i = 0; i < 100000; i++) { - assertNotEquals(-1, dict.getIdFromValue("t3_" + i)); + Assert.assertNotEquals(-1, dict.getIdFromValue("t3_" + i)); } - assertEquals(110011, dict.getIdFromValue("success")); + Assert.assertEquals(110011, dict.getIdFromValue("success")); + } + + @Test + public void testBuildGlobalDictFailed() throws IOException { + thrown.expect(IOException.class); + thrown.expectMessage("read failed."); + + GlobalDictionaryBuilder builder = new GlobalDictionaryBuilder(); + try { + DictionaryGenerator.buildDictionary(builder, dictionaryInfo, new ErrorDictionaryValueEnumerator()); + } catch (Throwable e) { + DistributedLock lock = KylinConfig.getInstanceFromEnv().getDistributedLockFactory().lockForCurrentThread(); + String lockPath = "/dict/" + dictionaryInfo.getSourceTable() + "_" + dictionaryInfo.getSourceColumn() + + "/lock"; + Assert.assertFalse(lock.isLocked(lockPath)); + throw e; + } } private class SharedBuilderThread extends Thread { @@ -118,4 +140,26 @@ public void run() { } } } -} + + private class ErrorDictionaryValueEnumerator implements IDictionaryValueEnumerator { + private int idx = 0; + + @Override + public String current() throws IOException { + return null; + } + + @Override + public boolean moveNext() throws IOException { + idx++; + if (idx == 1) + throw new IOException("read failed."); + return true; + } + + @Override + public void close() throws IOException { + + } + } +} \ No newline at end of file ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services