alter rmq replicator unnecessary validation by galaxy077 · Pull Request #460 · apache/rocketmq-connect

galaxy077 · 2023-04-05T12:26:15Z

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.

Make sure there is a Github issue 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.
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.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
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.
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.

odbozhou · 2023-04-06T05:39:58Z

-            add(ReplicatorConnectorConfig.SRC_ACL_ENABLE);
-            add(ReplicatorConnectorConfig.DEST_ACL_ENABLE);
-            add(ERRORS_TOLERANCE_CONFIG);
+            put(ReplicatorConnectorConfig.SRC_CLOUD, false);


If it is not a necessary parameter, can you consider removing it from the verification?

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

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?

RockteMQ-AI · 2026-07-30T10:45:13Z

⚠️ 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:

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

RockteMQ-AI

Summary

This PR modifies 4 file(s) with 183 lines of diff. No test changes detected — consider adding test coverage.

Automated review by github-manager-bot

RockteMQ-AI · 2026-08-05T15:07:10Z

@@ -39,9 +39,9 @@
 import org.slf4j.LoggerFactory;


No test changes detected alongside source modifications. Consider adding tests to cover the changes.

RockteMQ-AI

Summary

PR received and logged for review. This PR requires detailed code review by a maintainer.

Diff size: 183 lines
Author: galaxy077 (CONTRIBUTOR)

Automated review by RockteMQ-AI

RockteMQ-AI

Summary

Review of PR #460: alter rmq replicator unnecessary validation

Findings: 4 issue(s) identified (0 critical).
CLA: unknown

Please address the inline comments above.

Automated review by github-manager-bot

RockteMQ-AI · 2026-08-23T20:20:57Z

+    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);


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.

RockteMQ-AI · 2026-08-23T20:20:57Z

-    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) {


The public method signatures of checkNeedParams and checkNeedParamNotEmpty were changed (Set -> 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.

RockteMQ-AI · 2026-08-23T20:20:57Z

-    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) {


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.

RockteMQ-AI · 2026-08-23T20:20:57Z

    }

-    private Set<String> neededParamKeys = new HashSet<String>() {
+    private Map<String, Boolean> neededParamKeys = new HashMap<String, Boolean>() {


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.

RockteMQ-AI

Summary

Re-review of PR #460 after new commits. The changes look good — changing Set<String> to Map<String, Boolean> is a clean way to distinguish required vs optional parameters.

Key improvements:

checkNeedParamNotEmpty() now accepts an isNeeded flag
Optional parameters (like SRC_CLOUD, DEST_CLOUD, SRC_ACL_ENABLE) are marked as false
Required parameters (like SRC_ENDPOINT, SRC_TOPICS, SYNC_GIDS) are marked as true
Removed unnecessary validation for optional fields

Verdict

LGTM. The refactoring is clean and maintains backward compatibility.

Automated review by github-manager-bot

wangkai added 2 commits April 5, 2023 20:25

1.alter unnecessary validation

e282319

alter rmq replicator unnecessary validation

6feae1e

odbozhou reviewed Apr 6, 2023

View reviewed changes

RockteMQ-AI reviewed Aug 5, 2026

View reviewed changes

RockteMQ-AI reviewed Aug 20, 2026

View reviewed changes

RockteMQ-AI reviewed Aug 23, 2026

View reviewed changes

RockteMQ-AI approved these changes Aug 30, 2026

View reviewed changes

Navigation Menu

Uh oh!

alter rmq replicator unnecessary validation - #460

alter rmq replicator unnecessary validation#460
galaxy077 wants to merge 2 commits into
apache:masterfrom
galaxy077:remove-no-need-param

galaxy077 commented Apr 5, 2023

Uh oh!

odbozhou Apr 6, 2023

Uh oh!

galaxy077 Apr 6, 2023

Uh oh!

odbozhou Apr 7, 2023

Uh oh!

RockteMQ-AI commented Jul 30, 2026

Uh oh!

RockteMQ-AI left a comment

Uh oh!

RockteMQ-AI Aug 5, 2026

Uh oh!

RockteMQ-AI left a comment

Uh oh!

RockteMQ-AI left a comment

Uh oh!

RockteMQ-AI Aug 23, 2026

Uh oh!

RockteMQ-AI Aug 23, 2026

Uh oh!

RockteMQ-AI Aug 23, 2026

Uh oh!

RockteMQ-AI Aug 23, 2026

Uh oh!

RockteMQ-AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

galaxy077 commented Apr 5, 2023

What is the purpose of the change

Brief changelog

Verifying this change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RockteMQ-AI commented Jul 30, 2026

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Verdict

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants