G11n java client - Fix for expiration-related issues (issues 604, 664, and 662) #699

jessiejuachon · 2020-08-03T21:52:51Z

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-08-05T02:32:09Z

src/main/java/com/vmware/vipclient/i18n/base/cache/MessageCacheItem.java

-		if (cachedData != null) 
+	private final Map<String, String> cachedData = new HashMap<>();
+
+	public synchronized void setCacheItem(String locale, Map<String, String> cachedData, String etag, long timestamp, Long maxAgeMillis) {


Suggested change

public synchronized void setCacheItem(String locale, Map<String, String> cachedData, String etag, long timestamp, Long maxAgeMillis) {

public synchronized void setCacheItem(String locale, Map<String, String> dataToCache, String etag, long timestamp, Long maxAgeMillis) {

Xiaochao8 · 2020-08-05T03:08:38Z

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

-
-	    	cacheItem.addCachedData(messages);
-	    	cacheItem.setTimestamp(System.currentTimeMillis());
+			cacheItem.setCacheItem(dto.getLocale(), messages, null, System.currentTimeMillis(), null);


Don't need to store time to cache because data from local will never expire.

Data from local must expire. Use case: If data from local was fetched as a fallback for when Singleton service is temporarily down. In this case, when the cached data from local expires, it should try to fetch from the service again.

It should be set the time in other place because here is Local Opt, it doesn't need to consider the time.

So where do you recommend to set the time? By the way, there is nothing about expire time here, only timestamp of when the data was fetched from local bundles. And it has been there before this PR, just combined them into 1 call, so I do not want to make any change about that in this PR.

Xiaochao8 · 2020-08-05T03:12:38Z

src/main/java/com/vmware/vipclient/i18n/base/cache/MessageCacheItem.java

-	public synchronized void setMaxAgeMillis(Long maxAgeMillis) {
-		this.maxAgeMillis = maxAgeMillis;
-	}
+	public String getLocale() { return locale; }

 	public boolean isExpired() {


Suggested change

public boolean isExpired() {

public synchronized boolean isExpired() {

src/main/java/com/vmware/vipclient/i18n/messages/api/opt/server/ComponentBasedOpt.java

Xiaochao8 · 2020-08-05T03:18:43Z

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

@@ -96,7 +96,7 @@ public ComponentsService(final VIPCfg config) {
            final MessagesDTO dto = new MessagesDTO();
            dto.setComponent(comp);
            dto.setLocale(locale);
-            new CacheService(dto).addCacheOfComponent(new MessageCacheItem(messages, cacheItem.getEtag(), 
+            new CacheService(dto).addCacheOfComponent(new MessageCacheItem(locale, messages, cacheItem.getEtag(),


Here the etag is for all the components instead of a single component. so it's a wrong etag.

Right! Good catch. I'll set the etag to null for this one for now.
#646

Xiaochao8 · 2020-08-05T08:07:11Z

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

+					MessagesDTO fallbackLocaleDTO = new MessagesDTO(dto.getComponent(), cacheItemLocale, dto.getProductID(), dto.getVersion());
+					new ComponentService(fallbackLocaleDTO).fetchMessages(cacheItem, VIPCfg.getInstance().getMsgOriginsQueue().listIterator());


For some reasons, the requested locale failed last time. We still need to try the requested locale first, then fallback to default locale if failure.

I will update line 103 with this: if (cacheItem.isExpired() || !cacheItem.getLocale().equals(this.dto.getLocale())). It should resolve the issue.

I'm afraid it can't. If one locale gets the default locale's messages, it will always get the default locale's messages. It's wrong.

Right! My mistake. See the changes please

I mean that the requested locale fails not because locale isn't supported but other reasons such as network error. So next time to update cache, still need to get the requested locale first.

Yes, that is what is happening as I explained to Huihui.

In line 110, if the cache contains the fallback locale and not the requested locale, it will always try to fetch the requested locale first (line 113) and try to assign that to the requested locales cache key (line 116). Only if it fails again will it point to the fallback locale’s cacheItem again (line 122).

populateCacheTask only refreshes the content of the object that is already in the cache (e.g. either the target or fallback locale cacheItem), but it will not change the mapping of the object to the cache key/s. So this line you commented on will not affect the existing mapping/s of the object to cache key/s. It will just update the contents (the insides) of cacheItem object itself.

If you still do not get it, let's connect over Zoom

I have also updated the code to make it easier to understand, but same logic.

huihuiw01 · 2020-08-11T10:15:55Z

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

@@ -99,7 +99,8 @@ public MessageCacheItem getMessages(Iterator<Locale> fallbackLocalesIter) {
    	MessageCacheItem cacheItem = null;
    	if (cacheService.isContainComponent()) { // Item is in cache
    		cacheItem = cacheService.getCacheOfComponent();
-    		if (cacheItem.isExpired()) { // cacheItem has expired
+			// If the cacheItem is either expired or for a fallback locale
+    		if (cacheItem.isExpired() || !cacheItem.getLocale().equals(this.dto.getLocale())) {


With this condition !cacheItem.getLocale().equals(this.dto.getLocale()),
cache default locale's content for requested locale may not take effect, because they never be equal.

Xiaochao8 · 2020-08-13T02:41:39Z

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

+			// If the cacheItem is for a fallback locale, create and store cacheItem for the requested locale in a separate thread.
+    		if (!LocaleUtility.isSameLocale(cacheItem.getLocale(), this.dto.getLocale())) {
+				this.createCacheItemTask(fallbackLocalesIter);
 			}


How about moving this into above if block so it doesn't fetch every time.

We actually want to always try to fetch for the requested locale every time in a separate thread, not just when the cacheItem has expired. This is because if the cacheItem is for a fallback locale, any other call for a non-supported locale can trigger a refresh of the fallback locale's cacheItem. If that happens, the requested locale may not satisfy the if(cacheItem.isExpired() because the fallback locale has been refreshed by some other request. In this case, we still want to try and fetch the data for the requested locale.

Xiaochao8 · 2020-08-13T03:16:48Z

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

+		if (!cacheItem.getCachedData().isEmpty()) {
+			cacheService.addCacheOfComponent(cacheItem);
+		} else if (!dto.getLocale().equals(ConstantsKeys.SOURCE) && fallbackLocalesIter!=null && fallbackLocalesIter.hasNext()) {
+			// If failed to fetch message for the requetsed DTO, use MessageCacheItem of the next fallback locale.


Need to judge if current fallback locale is same as the locale in DTO. If same, should skip it.

If you follow the recursive logic, the locale in the dto will be the previous locale in the fallback locale queue.

Xiaochao8 · 2020-08-13T03:20:52Z

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

+	 * @param fallbackLocalesIter The fallback locale queue to use in case of failure.
+	 *
+	 */
+    private MessageCacheItem createCacheItem(Iterator<Locale> fallbackLocalesIter) {


It seems here has a recursive call: getMessages(Iterator fallbackLocalesIter) -> createCacheItem(Iterator fallbackLocalesIter) -> new ComponentService(fallbackLocaleDTO).getMessages(fallbackLocalesIter)

In my opinion, the hierarchical relation between methods are not clear.

huihuiw01 · 2020-08-13T06:24:34Z

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

+		} else if (!dto.getLocale().equals(ConstantsKeys.SOURCE) && fallbackLocalesIter!=null && fallbackLocalesIter.hasNext()) {
+			// If failed to fetch message for the requetsed DTO, use MessageCacheItem of the next fallback locale.
+			MessagesDTO fallbackLocaleDTO = new MessagesDTO(dto.getComponent(), fallbackLocalesIter.next().toLanguageTag(), dto.getProductID(), dto.getVersion());
+			cacheItem = new ComponentService(fallbackLocaleDTO).getMessages(fallbackLocalesIter);
+			if (!cacheItem.getCachedData().isEmpty()) {
+				cacheService.addCacheOfComponent(cacheItem);
+			}


Here only use the first fallback locale in the fallback locale chain, doesn't go through the whole fallback locale chain.

It actually does go through the whole cahin/queue. In line 138, you see fallbackLocalesIter.next(). By calling .next(), the current pointer of fallbackLocalesIter moves to the next item in the queue. When you pass fallbackLocalesIter to ComponentService.getMessages() in line139, the iterator holds the correct place (current pointer) in the queue.

…, and 662) (vmware#699) Fix for expiration-related issues (issues 604, 664, and 662) (vmware#699)

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 added 2 commits August 3, 2020 14:56

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

e63ab94

code clean up

ae1dccd

jessiejuachon marked this pull request as ready for review August 3, 2020 23:02

jessiejuachon requested review from Xiaochao8 and huihuiw01 August 3, 2020 23:02

jessiejuachon added 2 commits August 4, 2020 18:55

Mapping to the fallback locale's cache

6fbaab4

code clean up

ff99a37

Xiaochao8 reviewed Aug 5, 2020

View reviewed changes

changes after code review

271ee08

jessiejuachon requested a review from Xiaochao8 August 10, 2020 15:29

huihuiw01 reviewed Aug 11, 2020

View reviewed changes

jessiejuachon requested a review from huihuiw01 August 11, 2020 16:47

code clean up

230e21c

huihuiw01 previously approved these changes Aug 12, 2020

View reviewed changes

code clean up

55764a7

jessiejuachon dismissed huihuiw01’s stale review via 55764a7 August 12, 2020 21:05

jessiejuachon requested a review from huihuiw01 August 12, 2020 21:08

jessiejuachon added 3 commits August 12, 2020 14:47

code clean up

e9214cb

code clean up

beaa5ef

code clean up

673b8fd

Xiaochao8 reviewed Aug 13, 2020

View reviewed changes

Xiaochao8 approved these changes Aug 13, 2020

View reviewed changes

huihuiw01 reviewed Aug 13, 2020

View reviewed changes

huihuiw01 approved these changes Aug 14, 2020

View reviewed changes

jessiejuachon merged commit 8e5cd6f into vmware:g11n-java-client Aug 14, 2020

huihuiw01 pushed a commit to huihuiw01/singleton that referenced this pull request Aug 19, 2020

G11n java client - Fix for expiration-related issues (issues 604, 664…

7a03823

…, and 662) (vmware#699) Fix for expiration-related issues (issues 604, 664, and 662) (vmware#699)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

G11n java client - Fix for expiration-related issues (issues 604, 664, and 662) #699

G11n java client - Fix for expiration-related issues (issues 604, 664, and 662) #699

jessiejuachon commented Aug 3, 2020

Xiaochao8 Aug 5, 2020

Xiaochao8 Aug 5, 2020

jessiejuachon Aug 7, 2020

Xiaochao8 Aug 11, 2020

jessiejuachon Aug 11, 2020 •

edited

Loading

Xiaochao8 Aug 5, 2020

Xiaochao8 Aug 5, 2020

jessiejuachon Aug 7, 2020

Xiaochao8 Aug 5, 2020

jessiejuachon Aug 7, 2020 •

edited

Loading

Xiaochao8 Aug 11, 2020

jessiejuachon Aug 12, 2020

Xiaochao8 Aug 12, 2020

jessiejuachon Aug 12, 2020 •

edited

Loading

jessiejuachon Aug 12, 2020

huihuiw01 Aug 11, 2020

Xiaochao8 Aug 13, 2020

jessiejuachon Aug 13, 2020 •

edited

Loading

Xiaochao8 Aug 13, 2020

jessiejuachon Aug 13, 2020

Xiaochao8 Aug 13, 2020

huihuiw01 Aug 13, 2020

jessiejuachon Aug 13, 2020

	public synchronized void setCacheItem(String locale, Map<String, String> cachedData, String etag, long timestamp, Long maxAgeMillis) {
	public synchronized void setCacheItem(String locale, Map<String, String> dataToCache, String etag, long timestamp, Long maxAgeMillis) {

	public boolean isExpired() {
	public synchronized boolean isExpired() {

		MessagesDTO fallbackLocaleDTO = new MessagesDTO(dto.getComponent(), cacheItemLocale, dto.getProductID(), dto.getVersion());
		new ComponentService(fallbackLocaleDTO).fetchMessages(cacheItem, VIPCfg.getInstance().getMsgOriginsQueue().listIterator());

G11n java client - Fix for expiration-related issues (issues 604, 664, and 662) #699

G11n java client - Fix for expiration-related issues (issues 604, 664, and 662) #699

Conversation

jessiejuachon commented Aug 3, 2020

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 Aug 11, 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

jessiejuachon Aug 7, 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

jessiejuachon Aug 12, 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

jessiejuachon Aug 13, 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

jessiejuachon Aug 11, 2020 •

edited

Loading

jessiejuachon Aug 7, 2020 •

edited

Loading

jessiejuachon Aug 12, 2020 •

edited

Loading

jessiejuachon Aug 13, 2020 •

edited

Loading