G11n java client [BUG 754] Performance impact on the very first L3 fetch for a non-supported locale #823

jessiejuachon · 2020-10-13T21:07:54Z

No description provided.

…ation

https://sonarcloud.io/project/issues?id=jessiejuachon-java-client-g11n-java-client&issues=AXDG68NFL24bPpi-pS5s&open=AXDG68NFL24bPpi-pS5s https://sonarcloud.io/project/issues?id=jessiejuachon-java-client-g11n-java-client&issues=AXDG68IGL24bPpi-pS2r&open=AXDG68IGL24bPpi-pS2r Signed-off-by: Jessie <jessiejuachon@gmail.com>

…cache control

Signed-off-by: Jessie <jessiejuachon@gmail.com>

…and return them upstream.

…utAll(null)

…t in order to get a 304 NOT MODIFIED http response

…ne if component locale's cache has expired

…ingleton into g11n-java-client

…using cacheExpiredTime config

…g file. This means max age from server will be used. Setting VIPCfg.cacheExpiredTime to 0 disables caching.

…t need to be maintained.

Xiaochao8 · 2020-10-15T06:58:53Z

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

+	 * @param fallbackLocalesIter The fallback locale queue to use in case of failure. If null, no locale fallback will be applied.
+     */
+    private MessageCacheItem getFallbackLocaleMessages(Iterator<Locale> fallbackLocalesIter) {
+		if (!dto.getLocale().equals(ConstantsKeys.SOURCE) && fallbackLocalesIter != null && fallbackLocalesIter.hasNext()) {


Is it necessary to judge dto locale is same as the fallback locale?

It's not necessary, but it is an improvement. If we do not check for dto.getLocale == fallback and it happens, it will just try to fetch for the dto locale again and will fail again, and move on to the next item in the queue. Nonetheless, I have added the check for optimization.

Moreover, I hsve removed the following restriction to allow falling back to SOURCE messages:
!dto.getLocale().equals(ConstantsKeys.SOURCE)

Xiaochao8 · 2020-10-15T07:14:28Z

src/main/java/com/vmware/vipclient/i18n/util/LocaleUtility.java

+            if (!locales.contains(zhLocale) && !zhLocale.toLanguageTag().equals("zh-Hans") && !zhLocale.toLanguageTag().equals("zh-Hant")) {
+                return null;
+            }


Suggested change

if (!locales.contains(zhLocale) && !zhLocale.toLanguageTag().equals("zh-Hans") && !zhLocale.toLanguageTag().equals("zh-Hant")) {

return null;

}

if (preferredLocale.getLanguage().equals("zh")) {

Locale zhLocale = fmtToMappedLocale(preferredLocale);

if (!locales.contains(zhLocale)) {

String langTag = zhLocale.toLanguageTag();

if (langTag.equals("zh-Hans") || langTag.equals("zh-Hant")) {

return null;

}

}

}

I understand you want it to be more readable. There is incorrect logic in your suggestion though. It should be if (!langTag.equals("zh-Hans") && !langTag.equals("zh-Hant")) in line 160. Please review the new code.

src/main/java/com/vmware/vipclient/i18n/util/LocaleUtility.java

Xiaochao8 · 2020-10-20T04:11:03Z

src/main/java/com/vmware/vipclient/i18n/util/LocaleUtility.java

+        // For any Chinese locale (zh-*) that is not supported (except for zh-Hans and zh-Hant), use the fallback locale even if "zh" is supported.
+        if (preferredLocale.getLanguage().equals("zh")) {
+            Locale zhLocale = fmtToMappedLocale(preferredLocale);
+            if (!locales.contains(zhLocale)) {
+                String langTag = zhLocale.toLanguageTag();
+                if (!langTag.equals("zh-Hans") && !langTag.equals("zh-Hant"))
+                    return null;


Suggested change

// For any Chinese locale (zh-*) that is not supported (except for zh-Hans and zh-Hant), use the fallback locale even if "zh" is supported.

if (preferredLocale.getLanguage().equals("zh")) {

Locale zhLocale = fmtToMappedLocale(preferredLocale);

if (!locales.contains(zhLocale)) {

String langTag = zhLocale.toLanguageTag();

if (!langTag.equals("zh-Hans") && !langTag.equals("zh-Hant"))

return null;

// With Chinese locale which is not configured/supported in web.xml, it

// will return 'en_US' as default to meet the usage custom of Chinese,

// e.g. for 'zh-HK' from client(browser) which is not

// configured/supported yet, it will return 'en_US';

// Other locale, like 'de-DE' 'ja-JP' etc.,

// it will return 'de' 'ja'(main/parent language).

if (bestMatch != null && bestMatch.getLanguage().equals("zh")) {

if (!locales.contains(localeObject)) {

return null;

}

}

@Xiaochao8 , this most recent suggested change fails for test LocaleTest.testPickupLocaleFromListZh.
Isn't it supposed to return "zh" for any non-supported Chinese locale EXCEPT for zh-Hans/zh-Hant/zh-TW/zh-CN/zh-hans-CN/zh-Hant-TW?

I am not familiar with custom usage of Chinese, and so I will rely on you. I will make the suggested change and also update the test . Please double-check the expected behavior for me.

Co-authored-by: Xiaochao Li <48587632+Xiaochao8@users.noreply.github.com>

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

Xiaochao8 · 2020-10-26T03:37:04Z

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

+		this.doLocaleMatching();
+
+		CacheService cacheService = new CacheService(dto);
+		MessageCacheItem cacheItem = cacheService.getCacheOfComponent();
+		if (cacheItem != null) { // Item is in cache
+			if (cacheItem.isExpired())
+				refreshCacheItemTask(cacheItem); // Refresh the cacheItem in a separate thread
+		} else { // Item is not in cache.
 			ProductService ps = new ProductService(dto);
-			if (!ps.isSupportedLocale(Locale.forLanguageTag(dto.getLocale()))) { // Requested locale is not supported
-				Locale matchedLocale = LocaleUtility.pickupLocaleFromList(new LinkedList<>(ps.getSupportedLocales()), Locale.forLanguageTag(dto.getLocale()));
-				if (ps.isSupportedLocale(matchedLocale)) { // Requested locale matches a supported locale (eg. requested locale "fr_CA matches supported locale "fr")
-					MessagesDTO matchedLocaleDTO = new MessagesDTO(dto.getComponent(), matchedLocale.toLanguageTag(), dto.getProductID(), dto.getVersion());
-					return new ComponentService(matchedLocaleDTO).getMessages();
-				}
-			}
+			Locale locale = Locale.forLanguageTag(dto.getLocale());
+			if (ps.isSupportedLocale(locale) || VIPCfg.getInstance().isPseudo()) {
+				cacheItem = createCacheItem(); // Fetch for the requested locale from data store, create cacheItem and store in cache
+				if (cacheItem.getCachedData().isEmpty())  // Failed to fetch messages for the requested locale
+					cacheItem = getFallbackLocaleMessages(fallbackLocalesIter);
+			} else   // Requested locale is not supported and does not match any supported locales
+				cacheItem = getFallbackLocaleMessages(fallbackLocalesIter);
+		}
+		return cacheItem;


Suggest code for this method, then doLocaleMatching can be removed:

Locale matchedLocale = LocaleUtility.pickupLocaleFromList(new ProductService(dto).getSupportedLocales(), Locale.forLanguageTag(dto.getLocale())); MessageCacheItem cacheItem = null; if (matchedLocale != null) { dto.setLocale(matchedLocale.toLanguageTag()); cacheItem = new CacheService(dto).getCacheOfComponent(); if (cacheItem != null) { // Item is in cache if (cacheItem.isExpired()) refreshCacheItemTask(cacheItem); // Refresh the cacheItem in a separate thread } else { // Item is not in cache. cacheItem = createCacheItem(); // Fetch for the requested locale from data store, create cacheItem and store in cache } } if (cacheItem == null || cacheItem.getCachedData().isEmpty()) // Failed to fetch messages for the requested locale cacheItem = getFallbackLocaleMessages(fallbackLocalesIter); return cacheItem;

jessiejuachon and others added 30 commits March 10, 2020 17:00

Loading configuration from the JSON configuration file

f4c1214

Removing code that is not yet ready to be used from the sample applic…

1a4d95e

…ation

Adding a cache properties map to hold cache details such as etag and …

73d85ca

…cache control

Removing white space

2a38107

Signed-off-by: Jessie <jessiejuachon@gmail.com>

Fixing failing unit test

c7f5d4f

Parse headers, response code, response message from an HTTP response …

056891a

…and return them upstream.

Storing response headers in Map of cache properties

a2ba98b

Fixing failing unit test due to NullPointerException when doing Map.p…

17978bd

…utAll(null)

Changes after code review.

6c69170

Removing exception handling for now

41a8aa1

Deprecating VIPCfg.initialize; cleaning up code

dd2455d

Storing the http response code in the cache

e26d372

Using cached etag as if-none-match request header value in the reques…

01ab69e

…t in order to get a 304 NOT MODIFIED http response

Handling 404 response from Singleton service

750c9d2

Using HTTP repsonse's Cache-Control max-age and timestamp to dertermi…

529e5cc

…ne if component locale's cache has expired

Moving constants to URLUtils

b40fcf9

Fixing failing test

fa123f9

Fixing failing code scan

30b2054

Merge branch 'g11n-java-client' into g11n-java-client

0c69787

Cleaning up import in ComponentService,java

ffc0a74

Cleaning up code - caching/expiry

d8450e4

Merge branch 'g11n-java-client' of https://github.com/jessiejuachon/s…

d7f7641

…ingleton into g11n-java-client

Adding tests for new caching workflow; enabling old caching workflow …

0dcd827

…using cacheExpiredTime config

Fixing a failing test

d06adb3

Adding mock server response

48c8558

Removing unused file sampleconfig.json

4e45bed

Changes from code review

6a40dc8

Value of VIPCfg.cacheExpiredTime is -1 when value is not set in confi…

4c3c12d

…g file. This means max age from server will be used. Setting VIPCfg.cacheExpiredTime to 0 disables caching.

Changing a LinkedHashMap to a HashMap because insertion order does no…

7e0d939

…t need to be maintained.

more test cases

f8ce5c1

jessiejuachon marked this pull request as ready for review October 14, 2020 21:46

fixing intermittently failing test

c2b1fd9

jessiejuachon requested review from huihuiw01 and Xiaochao8 October 14, 2020 22:56

Xiaochao8 reviewed Oct 15, 2020

View reviewed changes

jessiejuachon requested a review from Xiaochao8 October 16, 2020 21:35

jessiejuachon added 3 commits October 16, 2020 14:53

code clean up for readability

6bbdfff

fixing incorrect if statement

d8e70b7

code optimization

7d8f943

Xiaochao8 reviewed Oct 20, 2020

View reviewed changes

jessiejuachon and others added 2 commits October 20, 2020 10:00

Update src/main/java/com/vmware/vipclient/i18n/util/LocaleUtility.java

b3bb30a

Co-authored-by: Xiaochao Li <48587632+Xiaochao8@users.noreply.github.com>

rolling back change

bdbae09

jessiejuachon requested a review from Xiaochao8 October 20, 2020 17:10

jessiejuachon added 3 commits October 20, 2020 12:17

changing Chinese locale matching rule

03bf400

Merge branch 'g11n-java-client' into g11n-java-client-754

71404e3

correcting incorrect merge

236ab56

Xiaochao8 previously approved these changes Oct 21, 2020

View reviewed changes

huihuiw01 suggested changes Oct 21, 2020

View reviewed changes

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

jessiejuachon requested a review from huihuiw01 October 21, 2020 21:18

Do Locale matching first in Componentservice.getMessages

7c2535c

jessiejuachon dismissed Xiaochao8’s stale review via 7c2535c October 23, 2020 16:34

jessiejuachon requested review from Xiaochao8 and removed request for Xiaochao8 October 23, 2020 16:35

huihuiw01 approved these changes Oct 26, 2020

View reviewed changes

Xiaochao8 reviewed Oct 26, 2020

View reviewed changes

Xiaochao8 approved these changes Oct 28, 2020

View reviewed changes

jessiejuachon merged commit 9a80266 into vmware:g11n-java-client Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

G11n java client [BUG 754] Performance impact on the very first L3 fetch for a non-supported locale #823

G11n java client [BUG 754] Performance impact on the very first L3 fetch for a non-supported locale #823

jessiejuachon commented Oct 13, 2020

Xiaochao8 Oct 15, 2020

jessiejuachon Oct 16, 2020

Xiaochao8 Oct 15, 2020

jessiejuachon Oct 16, 2020

Xiaochao8 Oct 20, 2020

jessiejuachon Oct 20, 2020 •

edited

Loading

Xiaochao8 Oct 26, 2020

G11n java client [BUG 754] Performance impact on the very first L3 fetch for a non-supported locale #823

G11n java client [BUG 754] Performance impact on the very first L3 fetch for a non-supported locale #823

Conversation

jessiejuachon commented Oct 13, 2020

Xiaochao8 Oct 15, 2020

Choose a reason for hiding this comment

jessiejuachon Oct 16, 2020

Choose a reason for hiding this comment

Xiaochao8 Oct 15, 2020

Choose a reason for hiding this comment

jessiejuachon Oct 16, 2020

Choose a reason for hiding this comment

Xiaochao8 Oct 20, 2020

Choose a reason for hiding this comment

jessiejuachon Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Xiaochao8 Oct 26, 2020

Choose a reason for hiding this comment

jessiejuachon Oct 20, 2020 •

edited

Loading