-
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
Loading offline resource bundles #511
Loading offline resource bundles #511
Conversation
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>
Signed-off-by: Jessie <jessiejuachon@gmail.com>
…and return them upstream.
…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.
@@ -48,6 +49,43 @@ public TranslationMessage() { | |||
super(); | |||
} | |||
|
|||
public String getMessage(final Locale locale, final String component, final SourceOpt sourceOpt, |
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.
Why was this method added?
Aren't we going to remove source-related things out of client SDK?
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.
2 use cases for using source messages:
- Source not collected yet
I think in some products, it is acceptable to deploy the product even if source messages have just been added/updated by developers, and available neither in Singleton service (online mode) nor offline bundles. (e.g. VMC deploys every 2 weeks if I am not mistaken. It can have new source strings that are not yet in the SaaS Singleton). In this case, the source message will be displayed. - isPseudo = true
That being said, only these 2 use cases use the source messages. #1 is not even applicable for on-prem products where translations have to be complete before release to production. #2 is not applicable in production.
On the other hand, there is no more source collection!
Also, the fallback mechanism (as seen in the new client workflow) uses the default locale instead of the source messages because the default locale may not be the same as the source locale. The implementation of this fallback mechanism is quite simple (about 10 lines of code in StringService.getString).
Note: I have updated the code with method comments for clarity.
src/main/java/com/vmware/vipclient/i18n/messages/api/opt/local/LocalMessagesOpt.java
Outdated
Show resolved
Hide resolved
…more; cleaning up tests; adding comments for javadoc
src/main/java/com/vmware/vipclient/i18n/base/instances/TranslationMessage.java
Outdated
Show resolved
Hide resolved
if (!source.equals(cachedSrcLocaleMsg) || | ||
cachedSrcLocaleMsg == null || cachedSrcLocaleMsg.isEmpty()) { | ||
return FormatUtils.format(source, sourceOpt.getLocale(), args); | ||
} |
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.
Still need to consider if isPseudo == true?
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.
Question.. why do we need to fetch the actual localized message if isPseudo is true? Isn't isPseudo just to show which keys use the client library? So isn't it enough to wrap the key like @@key@@?
src/main/java/com/vmware/vipclient/i18n/base/instances/TranslationMessage.java
Outdated
Show resolved
Hide resolved
|
||
} | ||
} | ||
return FormatUtils.format(message, locale, args); |
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.
Need to consider if adding ConstantsKeys.PSEUDOCHAR2?
Signed-off-by: Jessie <jessiejuachon@gmail.com>
@@ -86,6 +103,10 @@ public static synchronized VIPCfg getInstance() { | |||
} | |||
return gcInstance; | |||
} | |||
|
|||
public static synchronized void resetInstance() { |
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.
when this function will be called?
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.
See @before in CacheServiceTest.java and OfflineModeTest.java
VipCfg is different for each test so have to reset it.
I see that for other tests, you are manually putting configurations back to original as a workaround.
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 will change this.
src/main/java/com/vmware/vipclient/i18n/base/instances/TranslationMessage.java
Outdated
Show resolved
Hide resolved
src/main/java/com/vmware/vipclient/i18n/messages/service/StringService.java
Show resolved
Hide resolved
path = Paths.get(Thread.currentThread().getContextClassLoader(). | ||
getResource(path.toString()).toURI()); |
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.
The path should be relative to current working directory instead of Thread.currentThread().getContextClassLoader(), to make it easy to collect source.
Source collection will make use of config file.
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.
Let's make this change after this PR because it is related to source collection?
Also, source collection will NOT make use of config file.
@@ -53,6 +53,7 @@ public static synchronized I18nFactory getInstance(VIPCfg cfg) { | |||
if (factory == null) { | |||
factory = new I18nFactory(cfg); | |||
} | |||
factory.setCfg(cfg); |
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.
Need to remove this?
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.
Next PR
public void addMsgOriginsQueue(DataSourceEnum dataSource) { | ||
this.msgOriginsQueue.add(dataSource); | ||
} | ||
|
||
public void setMsgOriginsQueue(List<DataSourceEnum> msgOriginsQueue) { |
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.
Remove this 2 methods or make them private?
They don't need to exported.
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.
This is just a standard setter. This can be useful.
src/main/java/com/vmware/vipclient/i18n/base/cache/MessageCacheItem.java
Show resolved
Hide resolved
public synchronized void setCacheItem (MessageCacheItem cacheItem) { | ||
// Do not update cacheItem if timestamp is earlier than current. | ||
// An older timestamp comes from an old thread that was blocked. | ||
if (cacheItem.getTimestamp() < this.timestamp) | ||
return; |
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.
In this logic, we should prevent querying fro server parallelly.
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.
We can leave it if much effort.
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.
Try to merge, please.
No description provided.