-
Notifications
You must be signed in to change notification settings - Fork 740
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
Minimize reflection into Configuration #3127
Minimize reflection into Configuration #3127
Conversation
Pull Request Test Coverage Report for Build 5138
💛 - Coveralls |
* @param v the input argument | ||
* @param <V> the type of the input to the operation | ||
*/ | ||
public <V> void syncWriteConfiguration(ConfigurationValueConsumer<V> consumer, V v) { |
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'd swap the parameter order cause at it seems more natural to me like this
syncWriteConfiguration(groups, (c, g) -> {
populateGroups(g, new TreeSet<>(getProjects().values()));
c.setGroups(g);
});
as its "value -> consume" that value, rather then "consumer of a value -> value"
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.
That would mean a line like the following:
syncWriteConfiguration(flag, Configuration::setListDirsFirst);
which seems to invert the important part.
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'll rename the vague arguments and transpose it as you suggest
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.
thank you
} | ||
|
||
private ResourceLock newWriteUnlocker() { | ||
return () -> CloseableReentrantReadWriteLock.this.writeLock().unlock(); |
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 this.writeLock().unlock()
is not enough?
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.
Oh that was from the imported implementation. Yes it seems superfluous
public <V> void syncWriteConfiguration(ConfigurationValueConsumer<V> consumer, V v) { | ||
try (ResourceLock resourceLock = configLock.writeLockAsResource()) { | ||
//noinspection ConstantConditions to silence unreference auto-closeable | ||
assert resourceLock != null; |
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 do you assert this when you're explicitely checking this in write|readLockAsResource?
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.
That's to silent an annoying Java warning of the auto-closeable not being referenced in the block which was a blocker in IDEA when it runs tests with warnings-as-errors
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.
what about @SuppressWarnings("unused")
?
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 it’s a very focused warning for auto-closeable
} | ||
|
||
/** | ||
* @return a defined {@link ResourceLock} once the {@link #writeLock()} ()} |
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 are some extra brackets
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.
Got it
} | ||
}); | ||
if (capture[EXCEPTION_INDEX] != null) { | ||
throw capture[EXCEPTION_INDEX]; |
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 exception here is only translated to throw new WebApplicationException(e, Response.Status.BAD_REQUEST)
can you thro that one here and remove the try
block in the caller?
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.
Yes
} | ||
return result; | ||
} | ||
|
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.
extra line?
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 don't understand
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 don't know :-D I somehow saw two lines. Nevermind.
This is a very good idea with the configuration consumer! |
Thank you for taking a look, @tulinkry |
@ktulinger, were the changes satisfactory? |
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.
Sorry, I didn't submit approve! Here we go
Thank you, @tulinkry |
4e2b0c1
to
754ebdf
Compare
Just rebasing to ensure it still builds OK, since any new |
can Creative Commons Attribution-ShareAlike be merged to CDDL project? |
@tarzanek , yes I believe so. It's the license which Stack Overflow uses |
@tarzanek, see e.g. OGKTextField.java. |
in such case this can be merged, |
Thank you, Lubos! 😄 |
Hello,
Please consider for integration this patch to minimize reflection into
Configuration
and shift that logic toConfigurationController
where it's more difficult to avoid given the REST-exposure of all configuration through a single web method.RuntimeEnvironment
can use functional interfaces to achieve the equivalent of the former reflection without having to identify methods byString
; thereby we get compile-time checks.I also imported an auto-disposable wrapper around
Lock
so that try-with-resources can be used instead of try-finally.Thank you.