Skip to content

[rocketmq-replicator] Support create producer with connector's params… - #463

Open
casuallc wants to merge 2 commits into
apache:masterfrom
casuallc:feature_replicator
Open

[rocketmq-replicator] Support create producer with connector's params…#463
casuallc wants to merge 2 commits into
apache:masterfrom
casuallc:feature_replicator

Conversation

@casuallc

@casuallc casuallc commented Apr 6, 2023

Copy link
Copy Markdown

What is the purpose of the change

In source task, producer is create with default config in connect-standalone.conf.

This commit support use connector's config to create producer.

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.

liuchangqing added 2 commits April 6, 2023 16:16
# Conflicts:
#	connectors/rocketmq-replicator/src/main/java/org/apache/rocketmq/replicator/ReplicatorSourceTask.java
@casuallc

casuallc commented Apr 6, 2023

Copy link
Copy Markdown
Author

@odbozhou Please take a look.

@odbozhou

odbozhou commented Apr 6, 2023

Copy link
Copy Markdown
Contributor

In my opinion, this pr has some conflicts with the design of connect
Here is my opinion

  1. There are two kinds of connectors: source and sink. The destination of the source is the RocketMQ cluster where the current Connector Worker cluster is located
  2. Worker configuration should come from connect.conf, just like broker configuration comes from broker.conf
  3. The connector's own configuration should not affect the configuration of the Worker
  4. As part of the rocketmq capability, the replicator can synchronize messages from other rocketmq clusters to the current cluster

1 similar comment
@odbozhou

odbozhou commented Apr 6, 2023

Copy link
Copy Markdown
Contributor

In my opinion, this pr has some conflicts with the design of connect
Here is my opinion

  1. There are two kinds of connectors: source and sink. The destination of the source is the RocketMQ cluster where the current Connector Worker cluster is located
  2. Worker configuration should come from connect.conf, just like broker configuration comes from broker.conf
  3. The connector's own configuration should not affect the configuration of the Worker
  4. As part of the rocketmq capability, the replicator can synchronize messages from other rocketmq clusters to the current cluster

@casuallc

casuallc commented Apr 6, 2023

Copy link
Copy Markdown
Author

The connector is depolyed alone , so we should not bind to only one rocketmq cluster.
We can use the connector as a channel, user can config the source and dest.

And then what's the meaning to config dest.endpoint in rocketmq-replicator ? @odbozhou

@odbozhou

odbozhou commented Apr 7, 2023

Copy link
Copy Markdown
Contributor

dest.endpoint
dest.endpoint

The connector is depolyed alone , so we should not bind to only one rocketmq cluster. We can use the connector as a channel, user can config the source and dest.

And then what's the meaning to config dest.endpoint in rocketmq-replicator ? @odbozhou

The purpose of dest.endpoint is to allow the replicator to connect itself to the current cluster, so that the ability to automatically create a target topic or automatically create a consumerGroup can be realized.
For example, the replicator_checkpoint topic needs to be created in the checkpoint connector to the target rocketmq cluster.

In addition to the points mentioned above, it is also very important that the consumer side can use its perfect offset management mechanism to easily achieve at least one transmission. Because the producer logic is relatively simple, it can achieve at least one transmission in scenarios with poor network conditions. A transfer is more difficult. This is why replicator uses source to implement instead of sink.

@RockteMQ-AI

Copy link
Copy Markdown

⚠️ 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 feature_replicator
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 RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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


Automated review by github-manager-bot

@@ -16,12 +16,23 @@
*/

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Diff size: 354 lines
Author: casuallc (CONTRIBUTOR)


Automated review by RockteMQ-AI

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Review of PR #463: [rocketmq-replicator] Support create producer with connector's params…

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

Please address the inline comments above.


Automated review by github-manager-bot

@@ -699,9 +717,9 @@ public void start(KeyValue config) {
connectorConfig.setDestInstanceId(config.getString(ReplicatorConnectorConfig.DEST_INSTANCEID));
connectorConfig.setDestEndpoint(config.getString(ReplicatorConnectorConfig.DEST_ENDPOINT));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task still reads the destination endpoint from ReplicatorConnectorConfig.DEST_ENDPOINT, but ReplicatorSourceConnector now writes it under ConnectorConfig.RMQ_NAMESRVADDR. As a result, connectorConfig.getDestEndpoint() will be null in the task, which may break any logic that depends on the destination nameserver address.

*/
public final static String USE_NAMESRV_OF_CONNECTOR = "use.namesrv.of.connector";
public static final String RMQ_ACL_ENABLE = "rmq.acl.enable";
public static final String RMQ_ACCESS_KEY = "rmq.access.key";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RMQ_SECRET_KEY is defined as "rmq.secret_key" (underscore separator) while RMQ_ACCESS_KEY uses "rmq.access.key" (dot separator). This inconsistency is likely a typo and can cause user-provided secret keys to be ignored if they follow the dotted naming pattern of the other rmq.* keys.

@@ -111,21 +109,49 @@ public static AllocateConnAndTaskStrategy initAllocateConnAndTaskStrategy(Worker
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new producer initialization overload that reads connector-level ACL/namesrv/group settings is not covered by any visible test changes. Consider adding unit tests for both the use.namesrv.of.connector=true and false branches, including cases where keys are missing.

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Re-review of PR #463 after new commits. The changes look good — proper refactoring to support connector-specific producer configuration with clean fallback to worker defaults.

Key improvements in this PR:

  • Added USE_NAMESRV_OF_CONNECTOR flag to allow connectors to use their own namesrv address
  • Made neededParamKeys static (fixes potential memory leak)
  • Removed duplicate add(SRC_CLOUD) call
  • Changed Boolean.valueOf()/Long.valueOf() to parseBoolean()/parseLong() (better practice for primitives)
  • Backward-compatible overload of initDefaultMQProducer()

Findings

  • [Info] ConnectUtil.java:112 — Consider adding Javadoc for the new USE_NAMESRV_OF_CONNECTOR flag

Verdict

LGTM. The code changes are well-structured and maintain backward compatibility.


Automated review by github-manager-bot

}

public static DefaultMQProducer initDefaultMQProducer(WorkerConfig connectConfig) {
public static DefaultMQProducer initDefaultMQProducer(WorkerConfig connectConfig, ConnectKeyValue keyValue) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Info] The overloaded initDefaultMQProducer(WorkerConfig) method now delegates to the new method with null keyValue. This is a clean backward-compatible approach. Consider adding a brief Javadoc comment to explain the USE_NAMESRV_OF_CONNECTOR flag behavior for future maintainers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants