-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[g11n-java-client] Fix for issue #717 - non-supported locales trigger a request to VIP service in a separate thread for every getMessage/getMessages request #738
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI fails, could you check/
@Xiaochao8, it seems like the new license header check is in place even if your PR that updates the year hasn't been merged? Anyway, I have updated for those 2 files. |
MessagesDTO fallbackLocaleDTO = new MessagesDTO(dto.getComponent(), cacheItem.getLocale(), dto.getProductID(), dto.getVersion()); | ||
cacheItem = new ComponentService(fallbackLocaleDTO).getMessages(null); | ||
} | ||
if (cacheItem.isExpired()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add an 'else' before if?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we still want to refresh expired cacheItem even if it is for the fallback locale (returned in line 115).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's necessary to add it because the cache of fallback locale will be refreshed in line 115 because of the call getMessages with default locale.
If no 'else', it will be refreshed twice, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I have updated it.
if (cacheItem.getCachedData().isEmpty()) { // This means that the data to be used is from a fallback locale. | ||
// If expired, try to first create and store cacheItem for the requested locale in a separate thread. | ||
if (cacheItem.isExpired()) | ||
this.createCacheItemTask(fallbackLocalesIter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For creating second time, it doesn't need to do fallback. How about using 'resreshCache' without locale fallback? This will improve performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed line 111 to this.createCacheItemTask(null);
Passing null means locale fallback will not be applied.
MessagesDTO fallbackLocaleDTO = new MessagesDTO(dto.getComponent(), cacheItem.getLocale(), dto.getProductID(), dto.getVersion()); | ||
cacheItem = new ComponentService(fallbackLocaleDTO).getMessages(null); | ||
} | ||
if (cacheItem.isExpired()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's necessary to add it because the cache of fallback locale will be refreshed in line 115 because of the call getMessages with default locale.
If no 'else', it will be refreshed twice, right?
No description provided.