[improve][functions]18532: JavaInstanceStarter support reading configs from file #5181
Conversation
imaffe
changed the title
imaffe
force-pushed
the
affe/use-file-to-read-properties
branch
from
54f6865 to
8b018f6
Compare
| @Parameter(names = "--config_file", description = "The config file for instance to use, default " | ||
| + "use coomand line args") | ||
| public String configFile = null; | ||
|
|
There was a problem hiding this comment.
- this option should be exclusive to other previous configs. User either set it or set others, but not both at the same time.
- there should be a manual check that all previously required fields are present (either in the configfile or provided by user.)
There was a problem hiding this comment.
- The two config scheme is not exclusive to each other. The users can specify part of the configs using command line and the rest using a config file, but the command line args has a higher priority than the file configs. .
- The previously required fields are checked in
validateRequiredConfigs();, it will be called after we read config values from command line and config files (if any)
| listConverter = StringConverter.class) | ||
| public String jarFile; | ||
| public String jarFile = null; |
There was a problem hiding this comment.
String default value is null already, no need to explicit set it.
There was a problem hiding this comment.
The reason I kept them explicitly null is because : I was not sure how will the JCommander set the default value for String (JVM sets default value to null, but not sure if JCommander follows the rule). I did an experiement that JCommader initialize the value to null as well , but I figured it might be more clear that we explicitly set the value to null to avoid ambiguity (JCommander documentation didn't mention what the default value for string is if not provided one). The null value here serves a purpose : to distinguish between if a value is set or not provided. But sure since we know the value will be set to null even we do not do so explicitly, we can simplify the statments~
| public Boolean useTls = null; | ||
|
|
||
| @Parameter(names = "--tls_allow_insecure", description = "Allow insecure tls connection\n") | ||
| public String tlsAllowInsecureConnection = Boolean.TRUE.toString(); | ||
| @Parameter(names = "--tls_allow_insecure", description = "Allow insecure tls connection\n", arity = 1) | ||
| public Boolean tlsAllowInsecureConnection = null; | ||
|
|
||
| @Parameter(names = "--hostname_verification_enabled", description = "Enable hostname verification") | ||
| public String tlsHostNameVerificationEnabled = Boolean.FALSE.toString(); | ||
| @Parameter(names = "--hostname_verification_enabled", description = "Enable hostname verification", arity = 1) |
There was a problem hiding this comment.
we should set a meaningful value for Boolean type variables (either true or false instead of null)
There was a problem hiding this comment.
The default value is set in another method useDefaultValueIfBothFileAndJCommanderNotProvided. Using null because I want to distinguish between "not provided" and "provided a value but value is false"
imaffe
force-pushed
the
affe/use-file-to-read-properties
branch
2 times, most recently
from
dd74939 to
93ab5ee
Compare
imaffe
force-pushed
the
affe/use-file-to-read-properties
branch
2 times, most recently
from
3a05b0e to
4cda499
Compare
imaffe
force-pushed
the
affe/use-file-to-read-properties
branch
7 times, most recently
from
a7b1ac8 to
6cece3e
Compare
fix checkstyle not using * import 1. trigger CI try do not change the vairable type skip file ignore unit test do not use = null and skip set default value default value should be null uncomment file reader make file config and commandline config exclusive change the configuration logic remove TODO
imaffe
force-pushed
the
affe/use-file-to-read-properties
branch
from
6cece3e to
b351e0e
Compare
imaffe
force-pushed
the
affe/use-file-to-read-properties
branch
from
250c41d to
39d63e7
Compare
Fixes apache#18532
Master Issue: apache#18532
Motivation
Reading config options from command line is not very flexible in a cloud-native environment. For example, in distroless containers where there is no shell access, it's hard to determine a config value from env vars.
Using file to read the configs allow a more flexible configuration pattern.
Modifications
JavaInstanceConfiguration extends PulsarConfigurationto support reading configs from file.Verifying this change
(Please pick either of the following options)
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: