Non-blocking getSupportedLocales + Use getResourceAsStream to read resource bundle #878

jessiejuachon · 2020-11-17T17:55:34Z

For the reviewers:

Issues described in #882:

The java client library tries to walk the file system when getting the list of supported locales. This fails when the file system is in a nested jar. Such is the case in Spring boot applications where the offline bundles directory may exist inside dependency jars which under BOOT-INV/libs. This is related to [ENHANCEMENT and BUG] Non-blocking getSupportedLocales when fetching messages #877
The java client library tries to access the file system when reading the bundle (code is in LocalMessagesOpt.getComponentMessages). This fails when the file system is in a nested jar.

All files changed in this PR are for # 1 above, except for change sin LocalMessagesOpt.java which is for # 2.

…into non-block-cache

Xiaochao8 · 2020-12-04T07:24:14Z

src/main/java/com/vmware/vipclient/i18n/messages/service/ComponentService.java

+		dto.setLocale(LocaleUtility.fmtToMappedLocale(dto.getLocale()).toLanguageTag());
+
+		ProductService ps = new ProductService(dto);
+
+		//Refresh the cache of supported locales as needed in a separate thread (non-blocking).
+		ps.refreshSupportedLocalesTask();
+
+		//Match against list of supported locales that is already in the cache
+		Set<Locale> supportedLocales = LocaleUtility.langTagtoLocaleSet(ps.getCachedSupportedLocales());
+		Locale matchedLocale = LocaleUtility.pickupLocaleFromList(supportedLocales, Locale.forLanguageTag(dto.getLocale()));
+		if (matchedLocale != null) { // Requested locale matches a supported locale (eg. requested locale "fr_CA matches supported locale "fr")
 			dto.setLocale(matchedLocale.toLanguageTag());
+		}


I think the flow should as below:
Get supported locales from product service. Returning null if cache is empty. The product service will refresh cache.
If supported locales isn't empty, then pick up locale from them.

Current implementation is a little messy, hard to read and maintain.

Looks pretty much the same to me. Based on the flow you described

ComponentService will get supported locales from product service
This PR does it when ComponentService.doLocaleMatching calls ps.getCachedSupportedLocales().

The product service returns an empty set if cache of locales is not found.
ProductService.getCachedSupportedLocales() in # 1 returns an empty set if not found in cache.

The Product Service will refresh the cache if needed
Sure, ProductService.getCachedSupportedLocales now refreshes the cache in a separate thread instead of doing it separately here.

If supported locales set isn't null/empty, then pick up the locale from them.
This PR does it in ComponentService.doLocaleMatching after step 2: dto.setLocale(matchedLocale.toLanguageTag());

As 99% of the flow matches (only #3 needs some update, I am not sure what you mean by "little messy, hard to read and maintain".

The only added logic to the flow is ComponentService.proceed(), which is only called when message bundle is not yet in cache or is expired. This is for performance enhancement. ComponentService.proceed calls ps.getCachedSupportedLocales(dataSource) to check if the requested locale (dto.getLocale()) is supported in that specific data source. This is so that if it is not supported in that data source, it won't even try to fetch the message bundle. This is important especially if that invalid fetch is to a remote VIP service which will affect performance.

Xiaochao8 · 2020-12-04T07:43:45Z

src/main/java/com/vmware/vipclient/i18n/messages/service/ProductService.java

+    private Set<String> getSupportedLocales() {
+        return getSupportedLocales(true);
+    }
+
+    public Set<String> getCachedSupportedLocales() {


The interfaces should be consistent.
Do you plan to create such interface for component module as getCachedMessages or create interface getCacheComponents for component list?
If cache is empty, just populate the cache. if not, then return cache data and refresh cache if it's expired.

Please review the other issue linked to this: #877:

We do not need getCachedXXX for other data. We only need the presence/absence of cached supported locales to be non-blocking.

getCachedSupportedLcoales is an exception because we use it to pre-determine if getComponentMessages must even proceed or not. The locale list is not the requested information. It is used for performance enhancement only: so that if locale is not supported, it won't even try to fetch the bundle, and it will directly serve fallback locale bundle. However, if this performance-related call to getSupportedLocales was not even previously cached, it defeats its purpose. Not having this list in the cache should not make the call to getComponentMessages wait. Why wait for a separate call to getSupportedLocales when it can send this very first call to getComponentMessages directly? Refreshing the locales list can be done simultaneously in a separate thread to benefit succeeding getComponentMessages calls, but it is of no benefit to the first call at this point.

More importantly, if getSupportedLocales previously failed (or keeps on failing like in Atlas because FileSystems.newFilesSystems walk does not work in nested jars for Java 11 or less), it should definitely not block getComponentMessages. Otherwise, localized messages cannot be served at all. This is an unacceptable bug. Hence, we use getCachedSupportedLocales to just search from cache and not have to wait to fetch supported locales from data source every time, which is expected to fail in this scenario.

It seems you created so much code just to work around an extreme situation, getSupportedLocales keeps fail. In this case, we should remove test supported locales because stability takes precendence over performance. Let users take charge of it. Get the supported locale before getting translation or bear the delay of unsupported locale. In fact this logic has been there before for a long time.

Xiaochao8 · 2020-12-04T08:23:23Z

src/main/java/com/vmware/vipclient/i18n/messages/service/ProductService.java

-            if (cacheItem.isExpired())
-                refreshLocalesCacheItemTask(cacheItem, dataSource);
+            if (cacheItem.isExpired()) {
+                synchronized (cacheItem) { // Allow only 1 thread to refresh the cacheItem at a time.


It seems a new update process can be started after previous one has started successfully. right?

Yes, and that is why inside the synchronized block, another if (cacheItem.isExpired()) is done so that it will not proceed with the update if previous thread has already refreshed the cacheItem.

Anyway, I have removed this part out of this PR to narrow down the scope. I will include it in another PR.

Xiaochao8 · 2020-12-04T08:24:59Z

src/main/java/com/vmware/vipclient/i18n/messages/service/ProductService.java

            return cacheItem.getCachedData().keySet();
        } else {
-            cacheItem = createLocalesCacheItem(dataSource);
+            // Allow only 1 thread to create the cacheItem.
+            synchronized (TranslationCacheManager.getInstance().getCache(VIPCfg.CACHE_L3)) {


Is this too coarse-grained, there is only one level3 cache globally. Does this mean that only one cache item can be created at the same time?

I agree. Any suggestions on what object to use as a lock that will be specific to the item that does not exist yet?

I have removed this part out of this PR to narrow down the scope. I will include it in another PR.

Xiaochao8 · 2020-12-04T08:28:19Z

src/main/java/com/vmware/vipclient/i18n/messages/service/ProductService.java

-    }
-
-    public Set<String> getSupportedLanguageTags() {
+    private Set<String> getSupportedLocales(boolean refreshCache) {


It's better to create 2 method, one is getSupportedLocales and another is refreshSupportedLocales to reduct ambiguity.

I have renamed this to combineSupportedLocales. The overloading should be clear. Below are the method signatures

getSupportedLocales() - gets all supported locales (from all data sources)

getSupportedLocales(dataSource) - gets supported locales from the specified data source

getCachedSupportedLocales() - gets all supported locales (from all data sources) from cache

getCachedSupportedLocales(dataSource) - gets supported locales from the specified data source from cache

private combineSupportedLocales (refreshCache) - combines the list of supported locales from all dataSources. Used in # 1 and # 3.

I have also updated javadoc for all methods.

Xiaochao8 · 2020-12-04T08:57:17Z

src/main/java/com/vmware/vipclient/i18n/messages/service/ComponentService.java

+		 * Do not block refreshCacheItem if set of supported locales is not in cache (i.e. supportedLocales.isEmpty()).
+		 * This happens either when cache is not initialized, OR previous attempts to fetch the set had failed.
+		 */
+		return (supportedLocales.isEmpty() || supportedLocales.contains(dto.getLocale()) || VIPCfg.getInstance().isPseudo());


Why proceed if pseudo is true?

This has been there since before this PR. I just moved the if-statement judgements from ComponentService.getMessage to here, so no change here.

Xiaochao8 · 2020-12-04T08:58:58Z

src/main/java/com/vmware/vipclient/i18n/messages/service/ProductService.java

+        return getSupportedLocales(true);
+    }
+
+    public Set<String> getCachedSupportedLocales() {


The only reason we need this interface is that supported locales isn't cached. Suggest cache it when initialization. Then all the logic will be same as other features.

I also agree cache locale list when initialization no matter initializeCache is true or false, since it may reduce the chance of locale list fetching be triggered by message fetching which can improve performance at message fetching.
Further more, we may remove fetch locale list task code at message fetching.

This can be done in a new PR.

We should not force to cache this at app start up. This is why we have initializeCache property in the config. You may look at this as "small" data to always preload at start up time, but this is poor coding practice. We always lazy-load when possible. For instance whenever we use the Singleton design pattern, we do no even initialize the instance until it is needed (lazy initialization). Same should apply here.

Besides, why are you against caching the list in a separate thread only when requested instead of at app start up time? It calls the exact same code (ProductService.processSupportedLocales). We just added a logic to call createLocalesCacheItemTask in a separate thread if it needs to be non-blocking. Nothing else was changed in the existing logic.

In some cases, getSupportedLocales from offline resource bundle will always fail (FileSystems.newFileSystem walk fails in nested jar applications like in Atlas for Java 11 or less), so always calling getSupportedLocales that fails will affect performance as it tries to read from the disk (slow).

Please check ProductService. The code is now more unified.

src/main/java/com/vmware/vipclient/i18n/messages/service/ComponentService.java

huihuiw01 · 2020-12-04T15:49:31Z

src/main/java/com/vmware/vipclient/i18n/messages/service/ProductService.java

-            for (String languageTag : languageTags) {
-                locales.add(Locale.forLanguageTag(languageTag));
+
+    public void refreshSupportedLocalesTask() {


Actually when locale list is in cache, you won't populate it, so here 'refreshSupportedLocalesTask' may not appropriate, how about 'getSupportedLocalesTask'?

I have removed this task entirely. See code changes.

huihuiw01 · 2020-12-04T16:08:13Z

src/main/java/com/vmware/vipclient/i18n/messages/service/ProductService.java

+    private Set<String> getSupportedLocales() {
+        return getSupportedLocales(true);
+    }
+
+    public Set<String> getCachedSupportedLocales() {


There are 3 'getSupportedLocales' methods and 2 'getCachedSupportedLocales' methods, and they call each other, and seems 2 'getCachedSupportedLocales' are not the relation of overload, it's a little difficult to understand the relationship of these methods, it's better optimize the methods' name.

I have renamed 1 method only. The overloading should be clear. Look how the method signatures are self-explanatory.. descriptions seem to be repetitive of what is already obvious.

getSupportedLocales() - gets all supported locales (from all data sources)

getSupportedLocales(dataSource) - gets supported locales from the specified data source

getCachedSupportedLocales() - gets all supported locales (from all data sources) from cache

getCachedSupportedLocales(dataSource) - gets supported locales from the specified data source from cache

private combineSupportedLocales (refreshCache) - combines the list of supported locales from all dataSources. Used in # 1 and # 3.

I have also updated javadoc for all methods.

according OO thinking, the dataSource should be responsible of its methods instead of product service.

…ck-cache

Xiaochao8 · 2020-12-04T11:05:57Z

src/main/java/com/vmware/vipclient/i18n/messages/api/opt/local/LocalMessagesOpt.java

-				path = Paths.get(uri);
-				messages = JSONBundleUtil.getMessages(path);
-			}
+			InputStream is = getInputStream();


close the stream?

Xiaochao8 · 2020-12-05T13:51:19Z

src/main/java/com/vmware/vipclient/i18n/messages/service/ProductService.java

        Iterator<DataSourceEnum> msgSourceQueueIter = VIPCfg.getInstance().getMsgOriginsQueue().iterator();
        Set<String> supportedLangTags = new HashSet<>();
        while(msgSourceQueueIter.hasNext()) {
-            supportedLangTags.addAll(getSupportedLanguageTags(msgSourceQueueIter.next()));
+            supportedLangTags.addAll(getSupportedLanguageTags(withCacheRefresh, msgSourceQueueIter.next()));


As huihui said, the additional footprint isn't much because they are just a very few strings. But will improve performance greatly. Don't need to combine two set anymore.

Xiaochao8 · 2020-12-05T14:09:38Z

src/main/java/com/vmware/vipclient/i18n/messages/service/ProductService.java

+    private Set<String> getSupportedLocales() {
+        return getSupportedLocales(true);
+    }
+
+    public Set<String> getCachedSupportedLocales() {


It seems you created so much code just to work around an extreme situation, getSupportedLocales keeps fail. In this case, we should remove test supported locales because stability takes precendence over performance. Let users take charge of it. Get the supported locale before getting translation or bear the delay of unsupported locale. In fact this logic has been there before for a long time.

Xiaochao8 · 2020-12-05T14:13:56Z

src/main/java/com/vmware/vipclient/i18n/messages/service/ProductService.java

+    private Set<String> getSupportedLocales() {
+        return getSupportedLocales(true);
+    }
+
+    public Set<String> getCachedSupportedLocales() {


according OO thinking, the dataSource should be responsible of its methods instead of product service.

Xiaochao8 · 2020-12-05T14:30:00Z

src/main/java/com/vmware/vipclient/i18n/messages/api/opt/local/LocalMessagesOpt.java

+		while (true) {
+			String filePath = FormatUtils.format(OFFLINE_RESOURCE_PATH, dto.getComponent(), locale);
+			Path path = Paths.get(VIPCfg.getInstance().getOfflineResourcesBaseUrl(), filePath);
+			InputStream is = Thread.currentThread().getContextClassLoader().getResourceAsStream(path.toString());


Why change FileSystem to getResourceAsStream?

This is one of the primary root causes of the bug. Aside from getSupportedLocales is failing, and even if it succeeds, getComponentMessages is also failing! This is because FileSystems.newFileSystem fails in nested jars (not supported in Java 11 or below). However, getResourceAsStream does not fail! So getComponentMessages actually succeeds in this PR even for nested jars like in Atlas.

jessiejuachon · 2020-12-05T18:47:12Z

@Xiaochao8

"As huihui said, the additional footprint isn't much because they are just a very few strings. But will improve performance greatly. Don't need to combine two set anymore."

As I said, pre-loading anything is not good practice. Actually, pre-loading the list of supported locales will affect performance of application start up. How does it "improve performance greatly"? What's wrong with getting this only when needed in a separate thread? The added code is merely 5-7 lines of code (createLocalesCacheItemTask).

jessiejuachon · 2020-12-05T19:09:24Z

@Xiaochao8

It seems you created so much code just to work around an extreme situation, getSupportedLocales keeps fail. In this case, we should remove test supported locales because stability takes precendence over performance. Let users take charge of it. Get the supported locale before getting translation or bear the delay of unsupported locale. In fact this logic has been there before for a long time.

This "so much code" is NOT just a "workaround" for "an extreme situation". This is the proper behavior for any situation: The first step (getSupportedLocales) that is only used for performance purposes should not block the second step (getComponentMessages). This is regardless of whether FileSystems walk fails or not. ANY getSupportedLocales failure (offline or online mode) should not block getComponentMessages. For instance, getSupportedLocales from remote may fail too (e.g. temporary network issue), but getComponentMessages should still succeed if network issue is gone.

This is what this PR is about. It proves that even if getSupportedLocales step fails, messages can still be fetched.

The issue of FileSystem walk not working happens whenever the application has shared libs and are using Spring boot (or any platform that packages with nested jars instead of 1 fat jar). So this will be quite common. You are right, we should fix this issue for stability, but our hands are tied for now because FileSystem.newFilesSystem in nested jars is not supported in Java 11. We can explore other options later.

What do you mean by let users take charge of it? What can they do if getSupportedLocales failed?

Get the supported locale before getting translation or bear the delay of unsupported locale. In fact this logic has been there before for a long time.

You are mixing 2 issues:

There is a delay if supported locales is not yet fetched.
The only added code for this problem, is createLocalesCacheItemTask -- createLocalesCacheItem in a separate thread.
As I said, I do not see why fetching supported locales at app start up is better. It will affect app start up time performance. Fetching in a separate thread is merely 5-7 lines of code (createLocalesCacheItemTask) added.
getComponentMessages fails if first step (getSupportedLoclaes) failed.
This buggy logic has been there for a long time, but hidden for a while because we are not testing getSupportedLocales failed scenario. It was just made obvious by the Atlas bug. This is primarily what we are fixing in this PR (aside from using getResourceAsStream in getComponentMessages so that it will not fail even in nested jars)

jessiejuachon · 2020-12-05T19:17:43Z

@Xiaochao8

according OO thinking, the dataSource should be responsible of its methods instead of product service.

ProductService.getCachedSupportedLocales() is certainly not a function of the dataSource. The dataSource is not even processed here.

Getting supported locales from cache is a function of CacheService. CacheService is utilized when it is called down the line in ProductService.processSupportedLocales:

CacheService cs = new CacheService(new MessagesDTO(dto)); MessageCacheItem cacheItem = cs.getCacheOfLocales(dataSource);

Therefore, I do not think we are violating any OO concept.

Xiaochao8

Even though there are still some places to be improved, there aren't blocker issues. Merge it now.

jessiejuachon · 2020-12-07T17:55:25Z

Even though there are still some places to be improved, there aren't blocker issues. Merge it now.

I agree! Thanks for merging it. I have created the following to track the getSupportedLocales issue (FileSystems walk fails) in nested-jar applications: #912

Non-blocking getSupportedLocales

5f42099

jessiejuachon added kind/bug Something isn't working kind/enhancement Enhancement to existing features area/java-client priority/high labels Nov 17, 2020

jessiejuachon requested review from huihuiw01 and Xiaochao8 November 17, 2020 17:55

jessiejuachon self-assigned this Nov 17, 2020

jessiejuachon linked an issue Nov 17, 2020 that may be closed by this pull request

[ENHANCEMENT and BUG] Non-blocking getSupportedLocales when fetching messages #877

Closed

vmwclabot added the cla-not-required label Nov 17, 2020

jessiejuachon added 12 commits November 17, 2020 14:13

fixing failing test

a207056

code clean up

d291d7a

test

134d23d

fix test

a75c69b

code clean up

de5850a

code clean up

1c67db9

Refresh cache of supported locales in a separate thread

d7b27c3

code clean up

a9b5238

code clean up

e1dca83

fixing sonarcube bug

7f05a14

code cleanup

120e4d9

sonarcube bug

c0b385d

jessiejuachon linked an issue Nov 20, 2020 that may be closed by this pull request

[BUG] Java client offline mode fails in nested-jar applications such as Spring Boot (initially reported by the Atlas team) #882

Closed

jessiejuachon added 2 commits November 22, 2020 10:26

code clean up

12cc751

code clean up

d0dae5d

jessiejuachon removed a link to an issue Nov 22, 2020

[BUG] Java client offline mode fails in nested-jar applications such as Spring Boot (initially reported by the Atlas team) #882

Closed

jessiejuachon added priority/medium priority/high and removed priority/high priority/medium labels Nov 22, 2020

jessiejuachon added 4 commits December 3, 2020 13:38

changes after code review

e58155a

rename parameter

cebb186

code clean up

4eb761b

Merge branch 'non-block-cache' of github.com:jessiejuachon/singleton …

9fa2837

…into non-block-cache

jessiejuachon requested review from Xiaochao8 and huihuiw01 December 3, 2020 23:06

jessiejuachon added 4 commits December 3, 2020 19:38

thread safety

d6559b3

adding comment

5ab40c5

space clean up

26bc138

thread-safe file system access

f47feea

Xiaochao8 reviewed Dec 4, 2020

View reviewed changes

huihuiw01 previously approved these changes Dec 4, 2020

View reviewed changes

jessiejuachon mentioned this pull request Dec 4, 2020

Bug 868 [BUG] The offlineResourceBaseURL inside a shared lib is not being read if either main app is is run from IDE or executable jar has nested jars like in spring boot #869

Merged

optimizations

8f11aa8

jessiejuachon dismissed huihuiw01’s stale review via 8f11aa8 December 4, 2020 22:00

Merge remote-tracking branch 'upstream/g11n-java-client' into non-blo…

84b6e2f

…ck-cache

Xiaochao8 reviewed Dec 5, 2020

View reviewed changes

removing file system lock from this PR (out of scope)

1d830b2

jessiejuachon requested review from huihuiw01 and Xiaochao8 December 5, 2020 19:18

jessiejuachon added 2 commits December 5, 2020 11:30

closing input stream

b55e685

closing inputstream

d8ac6f6

huihuiw01 approved these changes Dec 6, 2020

View reviewed changes

Xiaochao8 approved these changes Dec 7, 2020

View reviewed changes

Xiaochao8 merged commit 8d7af99 into vmware:g11n-java-client Dec 7, 2020

Non-blocking getSupportedLocales + Use getResourceAsStream to read resource bundle #878

Non-blocking getSupportedLocales + Use getResourceAsStream to read resource bundle #878

Conversation

jessiejuachon commented Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

jessiejuachon Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessiejuachon Dec 4, 2020 • 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

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

Choose a reason for hiding this comment

jessiejuachon Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessiejuachon Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessiejuachon Dec 4, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessiejuachon Dec 5, 2020 • edited Loading

Choose a reason for hiding this comment

jessiejuachon commented Dec 5, 2020 • edited Loading

jessiejuachon commented Dec 5, 2020 • edited Loading

jessiejuachon commented Dec 5, 2020 • edited Loading

Xiaochao8 left a comment • edited Loading

Choose a reason for hiding this comment

jessiejuachon commented Dec 7, 2020 • edited Loading

jessiejuachon commented Nov 17, 2020 •

edited

Loading

jessiejuachon Dec 4, 2020 •

edited

Loading

jessiejuachon Dec 4, 2020 •

edited

Loading

jessiejuachon Dec 4, 2020 •

edited

Loading

jessiejuachon Dec 4, 2020 •

edited

Loading

jessiejuachon Dec 4, 2020 •

edited

Loading

jessiejuachon Dec 5, 2020 •

edited

Loading

jessiejuachon commented Dec 5, 2020 •

edited

Loading

jessiejuachon commented Dec 5, 2020 •

edited

Loading

jessiejuachon commented Dec 5, 2020 •

edited

Loading

Xiaochao8 left a comment •

edited

Loading

jessiejuachon commented Dec 7, 2020 •

edited

Loading