G11n java client code clean up - remove logic of adding a cacheItem with an empty dataMap to cache for non-supported locales #807

jessiejuachon · 2020-09-25T17:01:49Z

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.

jessiejuachon · 2020-09-25T17:05:12Z

@lyiyu66 , tagging you just for your information.

jessiejuachon · 2020-09-25T17:07:29Z

This is just the first step (code clean up) in solving 754. Here, I have removed the logic of adding a cacheItem with an empty dataMap to the cache for non-supported locales. We do not need this anymore because we are now first getting the list of supported locales to check if a locale is supported or not.

Next step is to use the fallback locale immediately to avoid fetch from service. This will be in another PR.

… into g11n-java-client-754

linr211 · 2020-09-28T03:23:31Z

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

+				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(null);


Seems recursive call is not good.

There is no recursive call in this line. Notice that getMessages is invoked on a separate instance of ComponentService. Could you be more specific as to what you mean by "not good"?

Also, this logic (lines 110-117) is not new. I just moved it out of the createCacheItem method that is called in line 120 into here for code clean up.

I meant line 115. 'not good' means that it gets the messages twice: one for supported locale, another or not supported locale. The logic is duplicated, the locale should be handled before call getMessages.

Secondly, when i read 'return new ComponentService(matchedLocaleDTO).getMessages(null);', i don't get what it means by passing a null value.

Reminder: This code has been there before this PR. It was for a different issue (#781), so this comment is out of the scope of this PR. Please do not let it block this PR.

The intention of this line is exactly what you mentioned. It is not a duplicate. If requested locale is not found in cache, we want to get a matched locale, if any, instead of the actual requested locale. It is the same strategy for getting messages for a fallback locale: new ComponentService(fallbackLocaleDTO).getMessages().
Part of the reason this looks messy is because the locale is currently part of the DTO. This is very old code that I did not want to change especially not in this PR as it is out of scope. In the future, I want to pass the locale to getMessages instead of having it in the DTO. If you do want to track this future improvement, please open another issue, because this PR is for code clean up only, not for changing any logic.

Secondly, passing a null here is unnecessary. I have changed it to getMessages().

Xiaochao8 · 2020-09-29T09:12:57Z

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

+				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(null);


It's obscure to pass null to getMessages. Hard to understand.

First, this has been there since before this PR, so based on suggestions from retrospective, we should open another issue to address this specific concern. This PR is for code clean up of caching an empty datamap only.

To understand the behavior when passing null to getMessages, you can read the javadoc of getMessages:

* @param fallbackLocalesIter The locale fallback queue to be used on failure. If null, there will be no fallback mechanism on failure so the message map will be empty.

I have logged the issue with description here: #811

Xiaochao8 · 2020-09-29T09:14:39Z

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

+				}
+			}
+
+			//Create and store cacheItem for the requested locale
 			cacheItem = createCacheItem(fallbackLocalesIter);


Is it necessary to pass fallbackLocalesIter? I think createCacheItem shouldn't be affected by fallbackLocalesIter.

You have already reviewed this code a long time ago though. Yes, it is necessary for now because I don't want to change any code logic in this PR. Please revisit this part in next PR.

Xiaochao8 · 2020-09-29T09:16:44Z

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

+			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")


Can't imagine getSupportedLocales is invoked three times in three code lines.

Where? It is only directly invoked in line 112. Do you mean ps.isSupportedLocale? Please clarify your comment (I don't understand what you can't imagine 😄 )

Note that there is formatting logic in ps.isSupportedLocale. For "cleaner" code, I put it in a separate method instead of having to call LocaleUtility.fmtToMappedLocale (which developers may forget to do in the future):

Set<Locale> supportedLocales = getSupportedLocales(); supportedLocales.contains(LocaleUtility.fmtToMappedLocale(...))

Let me know if you have abetter suggestion.

I think you can improve the LocaleUtility.pickupLocaleFromList to get enough information by only one call. Picking up once to get the supported locale or empty locale.

Ahhh yes! I agree. I already have that lined up. However, as I said, this PR is only to clean up / remove any code related to adding an empty data set into the cache.

Speaking of, I am also removing "locale" property from MessageCacheItem, so more changes have been added to this PR. This is actually related to this code clean up because "locale" was added for the purposes of adding an empty data map into cache for non-supported locales (which we are not doing anymore).

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

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

… into g11n-java-client

Removing locale parameter from MessageCacheItem

jessiejuachon · 2020-10-05T21:26:55Z

@Xiaochao8 , @huihuiw01 , please focus on the intent of the PR. There are already 12 files modified which is quite a lot, so please refrain from discussing changes that are not related to the PR's intent (i.e. clean up code that is related to caching an empty data map for non-supported locales).

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.

jessiejuachon requested a review from huihuiw01 September 25, 2020 17:02

jessiejuachon linked an issue Sep 25, 2020 that may be closed by this pull request

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

Closed

jessiejuachon added 5 commits September 25, 2020 15:23

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

3f488e0

… into g11n-java-client-754

code clean up

680aab4

code clean up

b10eec5

code clean up

610ffd0

Removing locale property from MessageCacheItem

01e8222

linr211 reviewed Sep 28, 2020

View reviewed changes

jessiejuachon requested a review from linr211 September 28, 2020 15:29

huihuiw01 previously approved these changes Sep 29, 2020

View reviewed changes

Xiaochao8 reviewed Sep 29, 2020

View reviewed changes

Update src/main/java/com/vmware/vipclient/i18n/messages/service/Compo…

e78a92e

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

jessiejuachon dismissed huihuiw01’s stale review via e78a92e September 29, 2020 15:51

jessiejuachon added 2 commits September 29, 2020 08:51

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

ce6cd77

… into g11n-java-client

changes based on review comments

138e1f9

jessiejuachon requested review from Xiaochao8 and huihuiw01 September 29, 2020 17:02

Enablling locale fallback for matched locale

b1a824a

jessiejuachon added the kind/enhancement Enhancement to existing features label Sep 29, 2020

jessiejuachon added 3 commits September 29, 2020 19:59

merging changes

6c4bb64

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

a4ddbdd

Merge pull request #1 from jessiejuachon/g11n-java-client-754

7b8bfaa

Removing locale parameter from MessageCacheItem

jessiejuachon removed the request for review from linr211 October 5, 2020 21:24

Xiaochao8 approved these changes Oct 12, 2020

View reviewed changes

jessiejuachon added the priority/medium label Oct 13, 2020

huihuiw01 approved these changes Oct 13, 2020

View reviewed changes

jessiejuachon merged commit 370e4e0 into vmware:g11n-java-client Oct 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

G11n java client code clean up - remove logic of adding a cacheItem with an empty dataMap to cache for non-supported locales #807

G11n java client code clean up - remove logic of adding a cacheItem with an empty dataMap to cache for non-supported locales #807

jessiejuachon commented Sep 25, 2020

jessiejuachon commented Sep 25, 2020 •

edited

Loading

jessiejuachon commented Sep 25, 2020

linr211 Sep 28, 2020

jessiejuachon Sep 28, 2020 •

edited

Loading

jessiejuachon Sep 28, 2020

linr211 Sep 29, 2020

jessiejuachon Sep 29, 2020 •

edited

Loading

Xiaochao8 Sep 29, 2020

jessiejuachon Sep 29, 2020 •

edited

Loading

Xiaochao8 Sep 29, 2020

jessiejuachon Sep 29, 2020 •

edited

Loading

Xiaochao8 Sep 29, 2020

jessiejuachon Sep 29, 2020 •

edited

Loading

Xiaochao8 Sep 30, 2020

jessiejuachon Oct 5, 2020 •

edited

Loading

jessiejuachon commented Oct 5, 2020

G11n java client code clean up - remove logic of adding a cacheItem with an empty dataMap to cache for non-supported locales #807

G11n java client code clean up - remove logic of adding a cacheItem with an empty dataMap to cache for non-supported locales #807

Conversation

jessiejuachon commented Sep 25, 2020

jessiejuachon commented Sep 25, 2020 • edited Loading

jessiejuachon commented Sep 25, 2020

linr211 Sep 28, 2020

Choose a reason for hiding this comment

jessiejuachon Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

jessiejuachon Sep 28, 2020

Choose a reason for hiding this comment

linr211 Sep 29, 2020

Choose a reason for hiding this comment

jessiejuachon Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

Xiaochao8 Sep 29, 2020

Choose a reason for hiding this comment

jessiejuachon Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

Xiaochao8 Sep 29, 2020

Choose a reason for hiding this comment

jessiejuachon Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

Xiaochao8 Sep 29, 2020

Choose a reason for hiding this comment

jessiejuachon Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

Xiaochao8 Sep 30, 2020

Choose a reason for hiding this comment

jessiejuachon Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

jessiejuachon commented Oct 5, 2020

jessiejuachon commented Sep 25, 2020 •

edited

Loading

jessiejuachon Sep 28, 2020 •

edited

Loading

jessiejuachon Sep 29, 2020 •

edited

Loading

jessiejuachon Sep 29, 2020 •

edited

Loading

jessiejuachon Sep 29, 2020 •

edited

Loading

jessiejuachon Sep 29, 2020 •

edited

Loading

jessiejuachon Oct 5, 2020 •

edited

Loading