-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(config): deprecate CLI params and add guardrails #6580
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
Changes from 1 commit
f55e9cf
84027da
5b0e37b
9d6027a
75b1d10
e197a03
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -998,6 +998,21 @@ private static void applyCLIParams(CLIParameter cmd, JCommander jc) { | |
| .map(ParameterDescription::getLongestName) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| jc.getParameters().stream() | ||
| .filter(ParameterDescription::isAssigned) | ||
| .filter(pd -> { | ||
| try { | ||
| return CLIParameter.class.getDeclaredField(pd.getParameterized().getName()) | ||
| .isAnnotationPresent(Deprecated.class); | ||
| } catch (NoSuchFieldException e) { | ||
| return false; | ||
| } | ||
| }) | ||
| .forEach(pd -> logger.warn( | ||
| "CLI option '{}' is deprecated and will be removed in a future release. " | ||
| + "Please use the corresponding config-file option instead.", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some deprecated CLI parameters (e.g.,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid point. Params like |
||
| pd.getLongestName())); | ||
|
|
||
| if (assigned.contains("--output-directory")) { | ||
| PARAMETER.outputDirectory = cmd.outputDirectory; | ||
| } | ||
|
|
@@ -1008,7 +1023,8 @@ private static void applyCLIParams(CLIParameter cmd, JCommander jc) { | |
| PARAMETER.supportConstant = cmd.supportConstant; | ||
| } | ||
| if (assigned.contains("--max-energy-limit-for-constant")) { | ||
| PARAMETER.maxEnergyLimitForConstant = cmd.maxEnergyLimitForConstant; | ||
| PARAMETER.maxEnergyLimitForConstant = max(3_000_000L, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that // In common/src/main/java/org/tron/core/Constant.java
public static final long ENERGY_LIMIT_IN_CONSTANT_TX = 3_000_000L;Would it make sense to reuse this constant here instead of the inline literal?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! Replaced both occurrences of |
||
| cmd.maxEnergyLimitForConstant, true); | ||
| } | ||
| if (assigned.contains("--lru-cache-size")) { | ||
| PARAMETER.lruCacheSize = cmd.lruCacheSize; | ||
|
|
@@ -1091,7 +1107,12 @@ private static void applyCLIParams(CLIParameter cmd, JCommander jc) { | |
| if (assigned.contains("--log-config")) { | ||
| PARAMETER.logbackPath = cmd.logbackPath; | ||
| } | ||
| // seedNodes is a JCommander positional (main) parameter, which does not support | ||
| // isAssigned(). An empty-check is used instead — this is safe because the default | ||
| // is an empty list, so non-empty means the user explicitly passed values on CLI. | ||
| if (!cmd.seedNodes.isEmpty()) { | ||
| logger.warn("Positional seed-node arguments are deprecated. " | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the feedback! Since |
||
| + "Please use seed.node.ip.list in the config file instead."); | ||
| List<InetSocketAddress> seeds = new ArrayList<>(); | ||
| for (String s : cmd.seedNodes) { | ||
| seeds.add(NetUtil.parseInetSocketAddress(s)); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,9 @@ | |
| * Fields here have NO default values — defaults live in CommonParameter. | ||
| * JCommander only populates fields that are explicitly passed on the | ||
| * command line. | ||
| * | ||
| * <p>Parameters marked {@code @Deprecated} are scheduled for removal. | ||
| * Use the corresponding config-file options instead.</p> | ||
| */ | ||
| @NoArgsConstructor | ||
| public class CLIParameter { | ||
|
|
@@ -44,63 +47,78 @@ public class CLIParameter { | |
| @Parameter(names = {"--password"}, description = "password") | ||
| public String password; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--solidity"}, description = "running a solidity node for java tron") | ||
| public boolean solidityNode; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SolidityNode.jar is no longer generated during the build process, but the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. |
||
|
|
||
| @Parameter(names = {"--keystore-factory"}, description = "running KeystoreFactory") | ||
| public boolean keystoreFactory; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--fast-forward"}) | ||
| public boolean fastForward; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--es"}, description = "Start event subscribe server") | ||
| public boolean eventSubscribe; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--p2p-disable"}, description = "Switch for p2p module initialization. " | ||
| + "(default: false)", arity = 1) | ||
| public boolean p2pDisable; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. |
||
|
|
||
| @Deprecated | ||
| @Parameter(description = "--seed-nodes") | ||
| public List<String> seedNodes = new ArrayList<>(); | ||
|
|
||
| // -- Storage parameters -- | ||
| // -- Storage parameters (deprecated, use config file instead) -- | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--storage-db-directory"}, description = "Storage db directory") | ||
| public String storageDbDirectory; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--storage-db-engine"}, | ||
| description = "Storage db engine.(leveldb or rocksdb)") | ||
| public String storageDbEngine; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--storage-db-synchronous"}, | ||
| description = "Storage db is synchronous or not.(true or false)") | ||
| public String storageDbSynchronous; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--storage-index-directory"}, description = "Storage index directory") | ||
| public String storageIndexDirectory; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--storage-index-switch"}, | ||
| description = "Storage index switch.(on or off)") | ||
| public String storageIndexSwitch; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--storage-transactionHistory-switch"}, | ||
| description = "Storage transaction history switch.(on or off)") | ||
| public String storageTransactionHistorySwitch; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--contract-parse-enable"}, description = "Switch for contract parses in " | ||
| + "java-tron. (default: true)") | ||
| public String contractParseEnable; | ||
|
|
||
| // -- Runtime parameters -- | ||
| // -- Runtime parameters (deprecated except --debug, use config file instead) -- | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--support-constant"}, description = "Support constant calling for TVM. " | ||
| + "(default: false)") | ||
| public boolean supportConstant; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--max-energy-limit-for-constant"}, | ||
| description = "Max energy limit for constant calling. (default: 100,000,000)") | ||
| public long maxEnergyLimitForConstant; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--lru-cache-size"}, description = "Max LRU size for caching bytecode and " | ||
| + "result of JUMPDEST analysis. (default: 500)") | ||
| public int lruCacheSize; | ||
|
|
@@ -109,48 +127,60 @@ public class CLIParameter { | |
| + "will not check for timeout. (default: false)") | ||
| public boolean debug; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--min-time-ratio"}, description = "Minimum CPU tolerance when executing " | ||
| + "timeout transactions while synchronizing blocks. (default: 0.0)") | ||
| public double minTimeRatio; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--max-time-ratio"}, description = "Maximum CPU tolerance when executing " | ||
| + "non-timeout transactions while synchronizing blocks. (default: 5.0)") | ||
| public double maxTimeRatio; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--save-internaltx"}, description = "Save internal transactions generated " | ||
| + "during TVM execution, such as create, call and suicide. (default: false)") | ||
| public boolean saveInternalTx; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--save-featured-internaltx"}, description = "Save featured internal " | ||
| + "transactions generated during TVM execution, such as freeze, vote and so on. " | ||
| + "(default: false)") | ||
| public boolean saveFeaturedInternalTx; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--save-cancel-all-unfreeze-v2-details"}, | ||
| description = "Record the details of the internal transactions generated by the " | ||
| + "CANCELALLUNFREEZEV2 opcode, such as bandwidth/energy/tronpower cancel amount. " | ||
| + "(default: false)") | ||
| public boolean saveCancelAllUnfreezeV2Details; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--long-running-time"}) | ||
| public int longRunningTime; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--max-connect-number"}, description = "Http server max connect number " | ||
| + "(default:50)") | ||
| public int maxHttpConnectNumber; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--rpc-thread"}, description = "Num of gRPC thread") | ||
| public int rpcThreadNum; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--solidity-thread"}, description = "Num of solidity thread") | ||
| public int solidityThreads; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--validate-sign-thread"}, description = "Num of validate thread") | ||
| public int validateSignThreadNum; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--trust-node"}, description = "Trust node addr") | ||
| public String trustNodeAddr; | ||
|
|
||
| @Deprecated | ||
| @Parameter(names = {"--history-balance-lookup"}) | ||
| public boolean historyBalanceLookup; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Consider enriching the warning message with the specific config-file key mapping so users know exactly how to migrate.
And if we implement this, it's necessary to add a static map like this:
And change this log like this:
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.
Good suggestion! Added a
DEPRECATED_CLI_TO_CONFIGmap that provides specific config-key hints in deprecation warnings.For params with a config equivalent:
CLI option '--storage-db-engine' is deprecated and will be removed in a future release. Please use config key 'storage.db.engine' instead.For params without a config equivalent (
--solidity,--p2p-disable):CLI option '--solidity' is deprecated and will be removed in a future release.This also addresses @3for's concern about misleading messages. See 5b0e37b.