You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "yiduwangkai (via GitHub)" <gi...@apache.org> on 2023/04/05 12:26:17 UTC
[GitHub] [rocketmq-connect] yiduwangkai opened a new pull request, #460: alter rmq replicator unnecessary validation
yiduwangkai opened a new pull request, #460:
URL: https://github.com/apache/rocketmq-connect/pull/460
## What is the purpose of the change
XXXXX
## Brief changelog
XX
## Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily. Notice, `it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR`.
- [x] Make sure there is a [Github issue](https://github.com/apache/rocketmq-connect/issues) filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
- [x] Format the pull request title like `[ISSUE #123] Fix UnknownException when host config not exist`. Each commit in the pull request should have a meaningful subject line and body.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in [test module](https://github.com/apache/rocketmq/tree/master/test).
- [x] Run `mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle` to make sure basic checks pass. Run `mvn clean install -DskipITs` to make sure unit-test pass. Run `mvn clean test-compile failsafe:integration-test` to make sure integration-test pass.
- [ ] If this contribution is large, please file an [Apache Individual Contributor License Agreement](http://www.apache.org/licenses/#clas).
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-connect] odbozhou commented on a diff in pull request #460: alter rmq replicator unnecessary validation
Posted by "odbozhou (via GitHub)" <gi...@apache.org>.
odbozhou commented on code in PR #460:
URL: https://github.com/apache/rocketmq-connect/pull/460#discussion_r1159303649
##########
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>() {
{
- add(ReplicatorConnectorConfig.SRC_CLOUD);
- add(ReplicatorConnectorConfig.SRC_REGION);
- add(ReplicatorConnectorConfig.SRC_CLUSTER);
- add(ReplicatorConnectorConfig.SRC_ENDPOINT);
- add(ReplicatorConnectorConfig.SRC_TOPICTAGS);
- add(ReplicatorConnectorConfig.DEST_CLOUD);
- add(ReplicatorConnectorConfig.DEST_REGION);
- add(ReplicatorConnectorConfig.DEST_CLUSTER);
- add(ReplicatorConnectorConfig.DEST_ENDPOINT);
- add(ReplicatorConnectorConfig.DEST_TOPIC);
- add(ReplicatorConnectorConfig.SRC_CLOUD);
- add(ReplicatorConnectorConfig.SRC_ACL_ENABLE);
- add(ReplicatorConnectorConfig.DEST_ACL_ENABLE);
- add(ERRORS_TOLERANCE_CONFIG);
+ put(ReplicatorConnectorConfig.SRC_CLOUD, false);
Review Comment:
If it is not a necessary parameter, can you consider removing it from the verification?
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] alter rmq replicator unnecessary validation [rocketmq-connect]
Posted by "RockteMQ-AI (via GitHub)" <gi...@apache.org>.
RockteMQ-AI commented on PR #460:
URL: https://github.com/apache/rocketmq-connect/pull/460#issuecomment-5129831271
⚠️ **Merge conflict detected**
This PR has conflicts with the base branch and cannot be merged. Please rebase or merge the base branch into your branch and resolve the conflicts:
```bash
git fetch origin
git checkout remove-no-need-param
git rebase origin/main
# resolve conflicts, then:
git push --force-with-lease
```
This is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved.
---
*Automated notification by github-manager-bot*
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] alter rmq replicator unnecessary validation [rocketmq-connect]
Posted by "RockteMQ-AI (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] alter rmq replicator unnecessary validation [rocketmq-connect]
Posted by "RockteMQ-AI (via GitHub)" <gi...@apache.org>.
RockteMQ-AI commented on code in PR #460:
URL: https://github.com/apache/rocketmq-connect/pull/460#discussion_r3721706400
##########
connectors/rocketmq-replicator/src/main/java/org/apache/rocketmq/replicator/ReplicatorCheckpointConnector.java:
##########
@@ -39,9 +39,9 @@
import org.slf4j.LoggerFactory;
Review Comment:
No test changes detected alongside source modifications. Consider adding tests to cover the changes.
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-connect] yiduwangkai commented on a diff in pull request #460: alter rmq replicator unnecessary validation
Posted by "yiduwangkai (via GitHub)" <gi...@apache.org>.
yiduwangkai commented on code in PR #460:
URL: https://github.com/apache/rocketmq-connect/pull/460#discussion_r1159663063
##########
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>() {
{
- add(ReplicatorConnectorConfig.SRC_CLOUD);
- add(ReplicatorConnectorConfig.SRC_REGION);
- add(ReplicatorConnectorConfig.SRC_CLUSTER);
- add(ReplicatorConnectorConfig.SRC_ENDPOINT);
- add(ReplicatorConnectorConfig.SRC_TOPICTAGS);
- add(ReplicatorConnectorConfig.DEST_CLOUD);
- add(ReplicatorConnectorConfig.DEST_REGION);
- add(ReplicatorConnectorConfig.DEST_CLUSTER);
- add(ReplicatorConnectorConfig.DEST_ENDPOINT);
- add(ReplicatorConnectorConfig.DEST_TOPIC);
- add(ReplicatorConnectorConfig.SRC_CLOUD);
- add(ReplicatorConnectorConfig.SRC_ACL_ENABLE);
- add(ReplicatorConnectorConfig.DEST_ACL_ENABLE);
- add(ERRORS_TOLERANCE_CONFIG);
+ put(ReplicatorConnectorConfig.SRC_CLOUD, false);
Review Comment:
It is also possible to remove them directly, but it is easier for me to extend or customize them. It would be better to extract the required parameters into a unified public class rather than write them for each connector
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-connect] odbozhou commented on a diff in pull request #460: alter rmq replicator unnecessary validation
Posted by "odbozhou (via GitHub)" <gi...@apache.org>.
odbozhou commented on code in PR #460:
URL: https://github.com/apache/rocketmq-connect/pull/460#discussion_r1160502658
##########
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>() {
{
- add(ReplicatorConnectorConfig.SRC_CLOUD);
- add(ReplicatorConnectorConfig.SRC_REGION);
- add(ReplicatorConnectorConfig.SRC_CLUSTER);
- add(ReplicatorConnectorConfig.SRC_ENDPOINT);
- add(ReplicatorConnectorConfig.SRC_TOPICTAGS);
- add(ReplicatorConnectorConfig.DEST_CLOUD);
- add(ReplicatorConnectorConfig.DEST_REGION);
- add(ReplicatorConnectorConfig.DEST_CLUSTER);
- add(ReplicatorConnectorConfig.DEST_ENDPOINT);
- add(ReplicatorConnectorConfig.DEST_TOPIC);
- add(ReplicatorConnectorConfig.SRC_CLOUD);
- add(ReplicatorConnectorConfig.SRC_ACL_ENABLE);
- add(ReplicatorConnectorConfig.DEST_ACL_ENABLE);
- add(ERRORS_TOLERANCE_CONFIG);
+ put(ReplicatorConnectorConfig.SRC_CLOUD, false);
Review Comment:
In my opinion, after the connector is implemented, the necessary configuration has been determined. In the subsequent evolution and development, of course, it is possible to challenge the verification rules of these parameters, but it is a very low-frequency operation. For example, we have some differences now. However, after our discussion this time, after confirming those that are indeed required and those that are not, there may be basically no major changes.
I think what you said about unifying the necessary parameters is a good idea. However, each connector implementation may have different validation rules, not only necessary and non-essential parameters. Can you try to tidy up some parameters though? Let's do further discussions on specific parameters?
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org