[WIP] Feature/move settings ruby to java#12531
Conversation
583177f to
db55924
Compare
| if ("String".equals(rclass.getName())) { | ||
| return String.class; | ||
| } | ||
| if ("Array".equals(rclass.getName())) { | ||
| return ArrayList.class; | ||
| } | ||
| if ("Java::OrgLogstashUtil::ModulesSettingArray".equals(rclass.getName())) { | ||
| return ModulesSettingArray.class; | ||
| } | ||
| if ("Java::OrgLogstashUtil::CloudSettingId".equals(rclass.getName())) { | ||
| return CloudSettingId.class; | ||
| } | ||
| if ("Java::OrgLogstashUtil::CloudSettingAuth".equals(rclass.getName())) { | ||
| return CloudSettingAuth.class; | ||
| } | ||
| if ("Java::OrgLogstashUtil::TimeValue".equals(rclass.getName())) { | ||
| return TimeValue.class; | ||
| } | ||
| // this cover Boolean (Ruby doesn't have a class for it) and StringCoercible | ||
| if ("Object".equals(rclass.getName())) { | ||
| return Object.class; | ||
| } | ||
| if ("Integer".equals(rclass.getName())) { | ||
| return Integer.class; | ||
| } | ||
| throw new IllegalArgumentException("Cannot find matching Java class for: " + rclass.getName()); |
There was a problem hiding this comment.
could this be pulled up into the specific SettingOption implementation referenced above? I'd like to avoid an ever-growing if chain. If we do leave the implementation here, perhaps a switch statement would be more efficient and read more clearly?
| if ("String".equals(rclass.getName())) { | |
| return String.class; | |
| } | |
| if ("Array".equals(rclass.getName())) { | |
| return ArrayList.class; | |
| } | |
| if ("Java::OrgLogstashUtil::ModulesSettingArray".equals(rclass.getName())) { | |
| return ModulesSettingArray.class; | |
| } | |
| if ("Java::OrgLogstashUtil::CloudSettingId".equals(rclass.getName())) { | |
| return CloudSettingId.class; | |
| } | |
| if ("Java::OrgLogstashUtil::CloudSettingAuth".equals(rclass.getName())) { | |
| return CloudSettingAuth.class; | |
| } | |
| if ("Java::OrgLogstashUtil::TimeValue".equals(rclass.getName())) { | |
| return TimeValue.class; | |
| } | |
| // this cover Boolean (Ruby doesn't have a class for it) and StringCoercible | |
| if ("Object".equals(rclass.getName())) { | |
| return Object.class; | |
| } | |
| if ("Integer".equals(rclass.getName())) { | |
| return Integer.class; | |
| } | |
| throw new IllegalArgumentException("Cannot find matching Java class for: " + rclass.getName()); | |
| switch (rclass.getName()) { | |
| case "String": | |
| return String.class; | |
| case "Array": | |
| return ArrayList.class; | |
| case "Java::OrgLogstashUtil::ModulesSettingArray": | |
| return org.logstash.util.ModulesSettingArray.class; | |
| case "Java::OrgLogstashUtil::CloudSettingId": | |
| return org.logstash.util.CloudSettingId.class; | |
| case "Java::OrgLogstashUtil::CloudSettingAuth": | |
| return org.logstash.util.CloudSettingAuth.class; | |
| case "Java::OrgLogstashUtil::TimeValue": | |
| return org.logstash.util.TimeValue.class; | |
| case "Object": | |
| return Object.class; | |
| case "Integer": | |
| return Integer.class; | |
| default: | |
| throw new IllegalArgumentException("Cannot find matching Java class for: " + rclass.getName()); | |
| } |
|
|
||
| private static final long serialVersionUID = 1L; | ||
|
|
||
| private Setting setting; |
There was a problem hiding this comment.
Thoughts:
While it might be a little more straight-forward here to just mirror our ruby setting in java-land, I think we have the opportunity to introduce a path toward safety.
In the end, I would prefer to have a collection of Setting objects that are immutable at runtime so that we can ensure that once they are initialized they are not modified. This will be especially useful as it will reduce the surface-area for bugs caused by plugins that reside in other code-bases modifying settings that they do not wholly own.
To achieve this, I think we need a few tweaks to the abstraction.
- Introduce java
SettingOption, an immutable object representing a single configurable setting name along with its defaults, validators, etc., but not the value. - Introduce java
Setting, an immutable object representing the current effective value of the setting and the information about whether that value was explicitly given or implicit. Implementation will likely carry a reference to a specificSettingOptioninstance. - Ruby
Settingremains mutable, but as a wrapper for the immutable javaSetting.- Its mutation methods (e.g.,
Setting#set(value),Setting#reset(value)) replace the reference to the new javaSetting. - Its mutation methods emit a deprecation log once the settings have been parsed from config and/or command-line. This will likely be implemented as a global variable that gets set once the config has been loaded.
- This bit could be implemented in vanilla ruby or in a jruby java extension.
- Its mutation methods (e.g.,
This will enable us to independently move setting initialization into java at a later time, and to subsequently remove the mutation methods from the ruby shim in a future major release.
There was a problem hiding this comment.
This PR is POC to move the head of settings hierarchy (the Setting class) from Ruby to Java. The idea is to create an isolation level (SettingExt + ProxyJavaSetting classes) to keep the 2 world separated to that we can try to improve the implementation on the Java side without affecting too much the Ruby one. Your idea to use value objects is great this saves from various side effects, however do we really have plugins that manipulate core settings?
There was a problem hiding this comment.
@yaauie I've a couple of questions.
- what do you mean for "whether that value was explicitly given or implicit."? Is implicit the value of the setting passed at construction time while the explicit is the one provided to the
setmethod? - if we think to use the existing Ruby Setting class as a container of the Java Setting (+ SettingOption) then we don't have any reason to create a JRuby extension for the root class Setting like this PR is doing, right?
| strict = args[3].toJava(Boolean.class); | ||
| } | ||
|
|
||
| System.out.println("DNADBG >> creating setting for " + name.asJavaString()); |
db55924 to
96604e2
Compare
…Java Setting class
…allow copy now it's deep
96604e2 to
68feccc
Compare
What does this PR do?
Why is it important/What is the impact to the user?
Checklist
Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs