RockteMQ-AI commented on code in PR #460:
URL: https://github.com/apache/rocketmq-connect/pull/460#discussion_r3839502895
##########
connectors/rocketmq-replicator/src/main/java/org/apache/rocketmq/replicator/utils/ReplicatorUtils.java:
##########
@@ -63,14 +63,14 @@ public static String buildConsumergroupWithNamespace(String
consumerGroup, Strin
return instanceId + "%" + consumerGroup;
}
- public static void checkNeedParams(String connectorName, KeyValue config,
Set<String> neededParamKeys) {
- for (String needParamKey : neededParamKeys) {
- checkNeedParamNotEmpty(connectorName, config, needParamKey);
+ public static void checkNeedParams(String connectorName, KeyValue config,
Map<String, Boolean> neededParamKeys) {
Review Comment:
The public method signatures of checkNeedParams and checkNeedParamNotEmpty
were changed (Set<String> -> Map<String, Boolean> and added boolean parameter).
This is a breaking API change for any external callers of these public utility
methods. If backward compatibility is required, consider adding overloaded
methods that preserve the old signatures.
##########
connectors/rocketmq-replicator/src/main/java/org/apache/rocketmq/replicator/utils/ReplicatorUtils.java:
##########
@@ -63,14 +63,14 @@ public static String buildConsumergroupWithNamespace(String
consumerGroup, Strin
return instanceId + "%" + consumerGroup;
}
- public static void checkNeedParams(String connectorName, KeyValue config,
Set<String> neededParamKeys) {
- for (String needParamKey : neededParamKeys) {
- checkNeedParamNotEmpty(connectorName, config, needParamKey);
+ public static void checkNeedParams(String connectorName, KeyValue config,
Map<String, Boolean> neededParamKeys) {
+ for (String needParamKey : neededParamKeys.keySet()) {
+ checkNeedParamNotEmpty(connectorName, config, needParamKey,
neededParamKeys.get(needParamKey));
}
}
- public static void checkNeedParamNotEmpty(String connectorName, KeyValue
config, String needParamKey) {
- if (StringUtils.isEmpty(config.getString(needParamKey, ""))) {
+ public static void checkNeedParamNotEmpty(String connectorName, KeyValue
config, String needParamKey, boolean isNeeded) {
+ if (StringUtils.isEmpty(config.getString(needParamKey, "")) &&
isNeeded) {
log.error("Replicator connector " + connectorName + " do not set "
+ needParamKey);
throw new ParamInvalidException("Replicator connector " +
connectorName + " do not set " + needParamKey);
Review Comment:
The ParamInvalidException message was shortened to only include the param
key, dropping the connector name and 'do not set' context. This makes error
messages less actionable and is inconsistent with the preceding log.error()
call that still logs the full message. Consider keeping the original full
exception message.
##########
connectors/rocketmq-replicator/src/main/java/org/apache/rocketmq/replicator/ReplicatorSourceConnector.java:
##########
@@ -208,22 +208,22 @@ public Class<? extends Task> taskClass() {
return ReplicatorSourceTask.class;
}
- private Set<String> neededParamKeys = new HashSet<String>() {
+ private Map<String, Boolean> neededParamKeys = new HashMap<String,
Boolean>() {
Review Comment:
No test changes are included for the altered validation behavior. Since this
changes which configuration keys are required versus optional for the source
connector, tests should cover both the required-field rejection path and the
newly-optional-field acceptance path.
##########
connectors/rocketmq-replicator/src/main/java/org/apache/rocketmq/replicator/utils/ReplicatorUtils.java:
##########
@@ -63,14 +63,14 @@ public static String buildConsumergroupWithNamespace(String
consumerGroup, Strin
return instanceId + "%" + consumerGroup;
}
- public static void checkNeedParams(String connectorName, KeyValue config,
Set<String> neededParamKeys) {
- for (String needParamKey : neededParamKeys) {
- checkNeedParamNotEmpty(connectorName, config, needParamKey);
+ public static void checkNeedParams(String connectorName, KeyValue config,
Map<String, Boolean> neededParamKeys) {
+ for (String needParamKey : neededParamKeys.keySet()) {
+ checkNeedParamNotEmpty(connectorName, config, needParamKey,
neededParamKeys.get(needParamKey));
}
}
- public static void checkNeedParamNotEmpty(String connectorName, KeyValue
config, String needParamKey) {
- if (StringUtils.isEmpty(config.getString(needParamKey, ""))) {
+ public static void checkNeedParamNotEmpty(String connectorName, KeyValue
config, String needParamKey, boolean isNeeded) {
+ if (StringUtils.isEmpty(config.getString(needParamKey, "")) &&
isNeeded) {
Review Comment:
With the new isNeeded flag, configuration parameters marked false skip
emptiness validation entirely. Ensure that every parameter marked false truly
has a safe runtime default or is genuinely optional; otherwise missing/empty
values may cause runtime failures downstream rather than failing fast during
validation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
