Skip to content
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

[FIXED JENKINS-39604] - ResourceBundleUtil#getBundle() should report misses on the low level #2627

Merged
merged 1 commit into from
Nov 9, 2016

Conversation

oleg-nenashev
Copy link
Member

Follow-up to the discussion in #2586 . This resource miss is a common and valid case for missing localizations, hence I propose to use the FINER level.

https://issues.jenkins-ci.org/browse/JENKINS-39604

@reviewbybees @scherler @olivergondza @daniel-beck

…resource misses on the low level

I propose the FINER level.
@oleg-nenashev oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Nov 8, 2016
@daniel-beck
Copy link
Member

Did we confirm, or are we certain, that this happens as regularly as I feared in the other PR?

If so, 👍

@ghost
Copy link

ghost commented Nov 9, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@olivergondza
Copy link
Member

@daniel-beck, I interpret it the same way. I expect authors would have an explicit reason to log something as high as WARNING but this seems like a simple mistake.

Copy link
Contributor

@scherler scherler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐝 Thanks @oleg-nenashev to follow up on that.

@oleg-nenashev
Copy link
Member Author

Did we confirm, or are we certain, that this happens as regularly as I feared in the other PR?

I think we're certain about it, but without jenkinsci/blueocean-plugin#556 we cannot reproduce it, I'd guess. Maybe @scherler could elaborate it

@oleg-nenashev
Copy link
Member Author

@reviewbybees done

@daniel-beck daniel-beck merged commit 72ee9e0 into jenkinsci:master Nov 9, 2016
@scherler
Copy link
Contributor

scherler commented Nov 9, 2016

@oleg-nenashev @daniel-beck in the original PR @tfennelly stated on the loop that it will be cached, so we only request it once.

that is because of https://github.com/scherler/jenkins/blob/46b483ed190fef1ac39f5e9624706faa20b8c1e4/core/src/main/java/jenkins/util/ResourceBundleUtil.java#L74-L78 and https://github.com/scherler/jenkins/blob/46b483ed190fef1ac39f5e9624706faa20b8c1e4/core/src/main/java/jenkins/util/ResourceBundleUtil.java#L50

Further in https://docs.oracle.com/javase/6/docs/api/java/util/ResourceBundle.html you will find

Cache Management

Resource bundle instances created by the getBundle factory methods are cached by default, and the factory methods return the same resource bundle instance multiple times if it has been cached. getBundle clients may clear the cache, manage the lifetime of cached resource bundle instances using time-to-live values, or specify not to cache resource bundle instances. Refer to the descriptions of the getBundle factory method, clearCache, ResourceBundle.Control.getTimeToLive, and ResourceBundle.Control.needsReload for details.

@jglick
Copy link
Member

jglick commented Nov 9, 2016

If and when getBundle returns a non-null value then that is indeed cached. But the case here is that getBundle returns null.

@scherler
Copy link
Contributor

scherler commented Nov 9, 2016

yeah, right. What would you recommend to overcome the later? One idea would be to store all installed plugins scan them and denote which matched and which not, to then only scan newly installed ones, but the question is: is it worth it?

@i386
Copy link

i386 commented Nov 9, 2016

Great to see this land 🎉

oleg-nenashev added a commit that referenced this pull request Nov 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-more-reviews Complex change, which would benefit from more eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants