-
Notifications
You must be signed in to change notification settings - Fork 49
Add generatedNameOf accepting package name and class name #63
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
base: master
Are you sure you want to change the base?
Conversation
|
cc @martint |
9664e44 to
2743958
Compare
| default String generatedNameOf(String packageName, String className) | ||
| { | ||
| return generatedNameOf(packageName, ImmutableMap.of("name", className)); | ||
| } | ||
|
|
||
| default String generatedNameOf(String packageName, String className, String name) | ||
| { | ||
| return generatedNameOf(packageName, ImmutableMap.of("type", className, "name", name)); | ||
| } | ||
|
|
||
| default String generatedNameOf(Class<?> type, Map<String, String> properties) | ||
| { | ||
| return generatedNameOf(type.getPackage().getName(), properties); | ||
| } | ||
|
|
||
| String generatedNameOf(String domain, Map<String, String> properties); |
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 no callers for these methods. How do you intend for these to be used?
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.
To generate a name for the class that was renamed and we want to keep backward compatibility providing previous name by hand
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 interface is meant to be used by the mbean exporter, not for users to call directly.
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.
How come?
newExporter(binder).export(DispatchManager.class).as(generator -> generator.generatedNameOf(QueryManager.class));
generator is of type ObjectNameGenerator
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 idea is to be able to do:
newExporter(binder).export(DispatchManager.class).as(generator -> generator.generatedNameOf("io.trino.execution", "QueryManager"));
to avoid a need to keep a class around just to get a package name and simple name from it
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've added a test
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.
@martint PTAL
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.
Ah, you're right. However, that's a pattern of usage I've been wanting to change. There's really no need to pass an object around when the caller can generate the names directly. Let me take a closer look at this PR to see if there's a better way to make it fit.
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.
Ok, let me know. Right now in Trino we are renaming beans to ensure backward compatibility of metrics
This is useful for scenarios when class was renamed and we want to use a fixed string for backward compatibility. Added tests for ObjectNameGenerator usage
5ee510c to
f8034a7
Compare
This is useful for scenarios when class was renamed and we want to use a fixed string for backward compatibility.