Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/923#discussion_r136669290
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
---
@@ -70,11 +69,13 @@
* </p>
*/
-public class SystemOptionManager extends BaseOptionManager implements
OptionManager, AutoCloseable {
+public class SystemOptionManager extends BaseOptionManager implements
AutoCloseable {
private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(SystemOptionManager.class);
- private CaseInsensitiveMap<OptionValidator> VALIDATORS;
- public void populateValidators() {
+ public static final CaseInsensitiveMap<OptionValidator>
DEFAULT_VALIDATORS =
--- End diff --
Agreed we should not be using global variables. The intended flow was the
following:
- DEFAULT_VALIDATORS holds the default set of validators
- The system option manager is passed a set of validators when it is
initialized
- The validators map is copied and stored in the validators variable on
line 230. So each option manager has a separate copy of the validator map.
Looking at this again I agree that this is not ideal. The
DEFAULT_VALIDATORS constant should be deleted and usages should be replaced
with createDefaultValidators.
I also see another issue that was present before where the validators
themselves are static, they should be explicitly copied, so we should probably
add a clone method to the validators to do that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
