Fix for dictionary synchronization issue #4

adrianocrestani · 2017-02-28T19:37:35Z

There is one (or two) issue when dictionaries are lazily initialized. The null verification that checks whether a dictionary has been initialized or not is not synchronized. That means that a dictionary may be loaded more than once, because the reference to it is not synced.

The other big issue is that on at least two of those dictionaries, the reference to the new dictionary escapes before it has completely been built. That leads to several problems when a second instance of the same dictionary is initialized while another thread is reading the first instance of that same dictionary.

The fix uses Guava's memoize to make sure that a dict is initialized only once and safely published. I was able to reproduce the bug and verify the fix with the code below.

        ExecutorService executorService = Executors.newFixedThreadPool(20);
        List<String> lines = FileUtils.readLines(new File("korean_strings_random.txt"), "UTF-8");

        for (final String line : lines) {
            executorService.submit(new Runnable() {
                @Override
                public void run() {
                    try {
                        KoreanTokenizer koreanTokenizer = new KoreanTokenizer();
                        KoreanFilter koreanFilter = new KoreanFilter(koreanTokenizer);
                        koreanTokenizer.setReader(new StringReader(line));
                        koreanFilter.reset();

                        while (koreanFilter.incrementToken()) ;
                    } catch (Throwable e) {
                        e.printStackTrace();
                    }
                }
            });
        }

Note that I did a very minimal effort to make those dictionary immutable, which would be ideal to avoid any future sync issue. I considered it out of the scope of this fix, since it wasn't required to fix the current dict initialization issues.

jpdaigle

Great start, let's review Guava dependency and some style issues.

jpdaigle · 2017-02-28T20:39:44Z

src/main/java/org/apache/lucene/analysis/kr/tagging/Tagger.java

@@ -21,6 +21,8 @@
 import java.util.Iterator;
 import java.util.List;

+import com.google.common.base.Supplier;
+import com.google.common.base.Suppliers;


I didn't see Guava in the maven pom file, how is it in scope as a dependency?

I couldn't find it out either, but it is there as compile time dependency when I run maven :/

jpdaigle · 2017-02-28T20:44:38Z

src/main/java/org/apache/lucene/analysis/kr/utils/DictionaryUtil.java

 import org.apache.lucene.analysis.kr.morph.CompoundEntry;
 import org.apache.lucene.analysis.kr.morph.MorphException;
 import org.apache.lucene.analysis.kr.morph.WordEntry;

 public class DictionaryUtil {

-	private static Trie<String,WordEntry> dictionary;
+	private static Supplier<Trie<String,WordEntry>> DICTIONARY = Suppliers.memoize(new Supplier<Trie<String, WordEntry>>() {


I understand "keep the style as the surrounding file" but not having spaces between the type params in the generic declaration is crazy! ಠ_ಠ

argh, my bad, I tried to fix that when I see them. I will fix it

jpdaigle · 2017-02-28T20:46:38Z

src/main/java/org/apache/lucene/analysis/kr/utils/DictionaryUtil.java

+        public Map<String, String> get() {
+            try {
+                return readDict(KoreanEnv.FILE_PREFIX);
+            } catch (MorphException e) {


seems like all the try/catch layout is repetitive and needs to be factored out into its own method?

hmm, good point, I will move that into readDict()

jpdaigle · 2017-02-28T20:46:49Z

src/main/java/org/apache/lucene/analysis/kr/utils/DictionaryUtil.java

+            List<String> lines;
+
+            try {
+                lines = FileUtil.readLines(KoreanEnv.getInstance().getValue(KoreanEnv.FILE_UNCOMPOUNDS),"UTF-8");


what formatting issue your seeing here?

jpdaigle · 2017-02-28T20:47:47Z

src/main/java/org/apache/lucene/analysis/kr/utils/DictionaryUtil.java

+                throw new RuntimeException("Failed to initialize UNCOMPOUNDS dictionary.", e);
+            }
+
+            for(String compound: lines) {


Needs some commenting, formatting, and cleanup - this block is messy and why do we need a custom StringUtil?

I just moved that block, so I tried to change as little as possible. If I start changing, I will probably change every line in that code. There is StringBuffer usage everywhere!!!

jpdaigle

Good news is: everything looks fine, and if you look at modern ES plugins (https://github.com/elastic/elasticsearch/blob/master/plugins/repository-hdfs/build.gradle) you'll see some of them actually use Guava, so we shouldn't get any pushback from the community for adding a guava dependency here.

The bad news: I think that before this merges, you need to align on guava versions, and fix the POM so that we understand how guava's getting pulled in. Remember that ES 2.3 will have a Guava version of "guava-18.0.jar". Somehow the ES5 hdfs plugin depends on guava 16 for compiling, so that doesn't seem right. I just reviewed the gradle build file for core ES, and it seems to just use Guava for tests (it's a test compile dependency only).

jpdaigle · 2017-02-28T23:57:40Z

src/main/java/org/apache/lucene/analysis/kr/utils/DictionaryUtil.java

@@ -312,12 +296,12 @@ public static String combineAndEomiCheck(char s, String eomi) throws MorphExcept
 	 * @param type	1: josa, 2: eomi
 	 * @throws MorphException


no longer true

The plugin depends on elasticsearch (2.3), which depends on guava 1.8. That is how guava jar is being added to the classpath at compile time. I don't see any change required here to fix the guava reference.

Wait are you replying to the "no longer true" because that's about the javadoc being outdated.

For the guava issue: yep, 18.0 but how is ES core depending on Guava 18.0? It's listed in their gradle build file as a test dependency only

fix the bad javadoc

merge it shipit

fix for synchronization issue on dictionaries initialization

e43b1e4

adrianocrestani requested review from jpdaigle and sforz February 28, 2017 19:37

jpdaigle reviewed Feb 28, 2017

View reviewed changes

changes based on JP's feedback

dcce308

jpdaigle reviewed Mar 1, 2017

View reviewed changes

removing outdated documentation

0b94a9c

adrianocrestani merged commit 2fdc7ea into tripadvisor:ES-2.3.5 Mar 1, 2017

adrianocrestani deleted the dict-sync-issue branch March 1, 2017 19:15

adrianocrestani mentioned this pull request Mar 1, 2017

Support for ES 2.3.5 chanil1218/elasticsearch-analysis-korean#11

Open

adrianocrestani mentioned this pull request Mar 28, 2017

ES 5.2.2 Support #5

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for dictionary synchronization issue #4

Fix for dictionary synchronization issue #4

adrianocrestani commented Feb 28, 2017 •

edited

Loading

jpdaigle left a comment

jpdaigle Feb 28, 2017

adrianocrestani Feb 28, 2017

jpdaigle Feb 28, 2017

adrianocrestani Feb 28, 2017

jpdaigle Feb 28, 2017

adrianocrestani Feb 28, 2017

jpdaigle Feb 28, 2017

adrianocrestani Feb 28, 2017 •

edited

Loading

jpdaigle Feb 28, 2017

adrianocrestani Feb 28, 2017

jpdaigle left a comment •

edited

Loading

jpdaigle Feb 28, 2017

adrianocrestani Mar 1, 2017

jpdaigle Mar 1, 2017

jpdaigle Mar 1, 2017

jpdaigle Mar 1, 2017 •

edited by adrianocrestani

Loading

		@@ -312,12 +296,12 @@ public static String combineAndEomiCheck(char s, String eomi) throws MorphExcept
		* @param type 1: josa, 2: eomi
		* @throws MorphException

Fix for dictionary synchronization issue #4

Fix for dictionary synchronization issue #4

Conversation

adrianocrestani commented Feb 28, 2017 • edited Loading

jpdaigle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianocrestani Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpdaigle left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpdaigle Mar 1, 2017 • edited by adrianocrestani Loading

Choose a reason for hiding this comment

adrianocrestani commented Feb 28, 2017 •

edited

Loading

adrianocrestani Feb 28, 2017 •

edited

Loading

jpdaigle left a comment •

edited

Loading

jpdaigle Mar 1, 2017 •

edited by adrianocrestani

Loading