-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Make git revision loading lazy #45358
Conversation
This commit makes the gitRevision property a lazy loaded value by returning an Object implementing toString(). The Dockerfile template is also changed to use groovy templates instead of the mavenfilter hack, so converting to String will not happen until runtime.
Pinging @elastic/es-core-infra |
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.
One comment otherwise the LGTM 👍
}); | ||
private Object createGitRevisionResolver(final Project project) { | ||
return new Object() { | ||
private final AtomicReference<String> gitRevision = new AtomicReference<>(); |
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.
Would it be better instead of making this an AtomicReference
to simply synchronize these calls so we can avoid to possibility shelling to git
multiple times if this is called concurrently? I think as it is we could do that and simply the first one would win.
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.
While this is a possibility, I wasn't sure it was necessary to avoid. I figured it was better to make concurrent calls fast, if they happen (but it seems unlikely?). By synchronizing the method, we would still be single threading the method even after we've retrieved the hash.
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.
Yeah, it's hard to say either way. I'm fine with leaving as is.
This commit makes the gitRevision property a lazy loaded value by returning an Object implementing toString(). The Dockerfile template is also changed to use groovy templates instead of the mavenfilter hack, so converting to String will not happen until runtime.
This commit makes the gitRevision property a lazy loaded value by returning an Object implementing toString(). The Dockerfile template is also changed to use groovy templates instead of the mavenfilter hack, so converting to String will not happen until runtime.
This commit makes the gitRevision property a lazy loaded value by
returning an Object implementing toString(). The Dockerfile template is
also changed to use groovy templates instead of the mavenfilter hack, so
converting to String will not happen until runtime.