-
Notifications
You must be signed in to change notification settings - Fork 120
rocketmq-replicator 同步消息异常 #145
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
base: master
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -30,11 +30,16 @@ | |
| import java.util.Collections; | ||
|
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. No test changes detected alongside source modifications. Consider adding tests to cover the changes. |
||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.HashMap; | ||
| import java.util.Set; | ||
| import java.util.Iterator; | ||
| import java.util.concurrent.TimeUnit; | ||
| import org.apache.rocketmq.client.exception.MQClientException; | ||
| import org.apache.rocketmq.common.admin.ConsumeStats; | ||
| import org.apache.rocketmq.common.admin.OffsetWrapper; | ||
| import org.apache.rocketmq.common.message.MessageQueue; | ||
| import org.apache.rocketmq.common.protocol.body.ClusterInfo; | ||
| import org.apache.rocketmq.common.protocol.route.BrokerData; | ||
| import org.apache.rocketmq.replicator.common.Utils; | ||
| import org.apache.rocketmq.replicator.config.ConfigUtil; | ||
| import org.apache.rocketmq.replicator.config.TaskConfig; | ||
|
|
@@ -52,6 +57,7 @@ public class MetaSourceTask extends SourceTask { | |
| private final String taskId; | ||
| private final TaskConfig config; | ||
| private DefaultMQAdminExt srcMQAdminExt; | ||
| private DefaultMQAdminExt tarMQAdminExt; | ||
| private volatile boolean started = false; | ||
|
|
||
| private OffsetSyncStore store; | ||
|
|
@@ -77,6 +83,7 @@ public void start(SourceTaskContext sourceTaskContext) { | |
|
|
||
| try { | ||
| this.srcMQAdminExt = Utils.startMQAdminTool(this.config); | ||
| this.tarMQAdminExt = Utils.startTarMQAdminTool(this.config); | ||
| } catch (MQClientException e) { | ||
| log.error("Replicator task start failed for `startMQAdminTool` exception.", e); | ||
| throw new IllegalStateException("Replicator task start failed for `startMQAdminTool` exception."); | ||
|
|
@@ -92,6 +99,7 @@ public void stop() { | |
| started = false; | ||
| } | ||
| srcMQAdminExt.shutdown(); | ||
| tarMQAdminExt.shutdown(); | ||
|
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. stop() calls tarMQAdminExt.shutdown() without a null guard. If start() throws after srcMQAdminExt started (e.g. startTarMQAdminTool failure), tarMQAdminExt is null and stop() NPEs, masking the real error, while the already-started srcMQAdminExt is never shut down (resource leak). Guard both shutdowns and release srcMQAdminExt when the target admin fails to start. |
||
| } | ||
|
|
||
|
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. NPE risk in stop(): |
||
| @Override | ||
|
|
@@ -120,28 +128,38 @@ public void resume() { | |
| List<ConnectRecord> res = new ArrayList<>(); | ||
|
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. No test coverage for the substantially changed meta offset sync logic (direct updateConsumeOffset, new startTarMQAdminTool, ClusterInfo traversal) or the RocketMQConverter send-to-specific-queue path in WorkerSourceTask. These are significant behavioral changes that warrant integration tests, especially given the NPE risks identified above. |
||
| for (String group : groups) { | ||
| ConsumeStats stats; | ||
| String brokerAddresMaster=""; | ||
| String brokerName=""; | ||
| try { | ||
| stats = this.srcMQAdminExt.examineConsumeStats(group); | ||
| ClusterInfo clusterInfo = this.tarMQAdminExt.examineBrokerClusterInfo(); | ||
|
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. Performance: 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. examineBrokerClusterInfo() is an expensive admin RPC invoked inside the per-group loop on every poll, and poll() is called in a tight runtime loop with no sleep, so cluster info is re-fetched G times per iteration. Hoist the call outside the groups loop and cache it (or refresh periodically); the nested group × broker × queue iteration can also be inverted so each queue is matched against a broker-name map. |
||
| HashMap<String, Set<String>> clusterAddrTable = clusterInfo.getClusterAddrTable(); | ||
| HashMap<String, BrokerData> brokerAddrTable = clusterInfo.getBrokerAddrTable(); | ||
| Set<String> clusterNameSet = clusterAddrTable.get(this.config.getTargetCluster()); | ||
|
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. NPE risk: |
||
| Iterator<String> it = clusterNameSet.iterator(); | ||
|
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.
|
||
| while (it.hasNext()){ | ||
|
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. Performance: The while-loop iterates all brokers in the target cluster, and for each broker iterates ALL message queues from the consume stats, only updating the offset when |
||
| String clusterName = it.next(); | ||
| BrokerData brokerData = brokerAddrTable.get(clusterName); | ||
|
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. NPE risk: |
||
| HashMap<Long, String> brokerAddrs = brokerData.getBrokerAddrs(); | ||
| brokerAddresMaster = brokerAddrs.get(new Long(0)); | ||
| brokerName = brokerData.getBrokerName(); | ||
| for (Map.Entry<MessageQueue, OffsetWrapper> offsetTable : stats.getOffsetTable().entrySet()) { | ||
| MessageQueue mq = offsetTable.getKey(); | ||
| long srcOffset = offsetTable.getValue().getConsumerOffset(); | ||
| long targetOffset = this.store.convertTargetOffset(mq, group, srcOffset); | ||
| try{ | ||
| if (brokerName.equals(mq.getBrokerName())){ | ||
|
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. Offset sync matches the TARGET broker name against the SOURCE MessageQueue broker name, and WorkerSourceTask similarly sends to the same queueId/brokerName — the whole change assumes the target cluster has identical broker names and at least as many queues per topic. When names or queue counts differ, offsets are silently skipped here and data sends throw MQClientException in the runtime (records dropped). Also brokerAddrs.get(new Long(0)) returns null when no master is registered, making updateConsumeOffset fail. Validate the target route exists and log a clear warning otherwise; use Long.valueOf(0)/MixAll.MASTER_ID instead of new Long(0). |
||
| this.tarMQAdminExt.updateConsumeOffset(brokerAddresMaster,group,mq,targetOffset); | ||
| } | ||
| }catch (Exception e){ | ||
| log.error("admin update consumer offset err", e); | ||
| } | ||
| } | ||
| } | ||
| } catch (Exception e) { | ||
| log.error("admin get consumer info failed for consumer groups: " + group, e); | ||
| continue; | ||
| } | ||
|
|
||
| for (Map.Entry<MessageQueue, OffsetWrapper> offsetTable : stats.getOffsetTable().entrySet()) { | ||
| MessageQueue mq = offsetTable.getKey(); | ||
| long srcOffset = offsetTable.getValue().getConsumerOffset(); | ||
| long targetOffset = this.store.convertTargetOffset(mq, group, srcOffset); | ||
|
|
||
| List<Field> fields = new ArrayList<Field>(); | ||
| Schema schema = new Schema(SchemaEnum.OFFSET.name(), FieldType.INT64, fields); | ||
| schema.getFields().add(new Field(0, FieldName.OFFSET.getKey(), SchemaBuilder.string().build())); | ||
|
|
||
| JSONObject jsonObject = new JSONObject(); | ||
| jsonObject.put(FieldName.OFFSET.getKey(), targetOffset); | ||
| ConnectRecord connectRecord = new ConnectRecord(Utils.offsetKey(mq), | ||
| Utils.offsetValue(srcOffset), System.currentTimeMillis(), schema, jsonObject.toJSONString()); | ||
| res.add(connectRecord); | ||
| } | ||
| } | ||
| return res; | ||
|
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. poll() now always returns an empty list — the record/schema machinery (ConnectRecord, Schema, Field, SchemaBuilder, JSONObject, FieldName, SchemaEnum imports, and the |
||
| } | ||
|
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. Offset tracking concern: poll() now always returns an empty ConnectRecord list, and sendRecord() returns early for RocketMQMetaConverter, bypassing positionStorageWriter entirely. The framework's position/offset tracking is completely skipped for meta tasks. On task restart or reassignment, no progress is recorded. Verify this is intentional — the direct updateConsumeOffset approach may need at-least-once delivery guarantees that the framework no longer provides for this path. |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
| import com.alibaba.fastjson.JSON; | ||
| import com.alibaba.fastjson.JSONObject; | ||
| import io.openmessaging.KeyValue; | ||
| import io.openmessaging.internal.DefaultKeyValue; | ||
| import io.openmessaging.connector.api.component.task.source.SourceTask; | ||
| import io.openmessaging.connector.api.component.task.source.SourceTaskContext; | ||
| import io.openmessaging.connector.api.data.ConnectRecord; | ||
|
|
@@ -178,6 +179,11 @@ private List<ConnectRecord> pollCommonMessage() { | |
| final Map<String, String> properties = msg.getProperties(); | ||
| final Set<String> keys = properties.keySet(); | ||
| keys.forEach(key -> connectRecord.addExtension(key, properties.get(key))); | ||
| connectRecord.addExtension("topic",taskTopicConfig.getTargetTopic()); | ||
|
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. Adding routing metadata as plain extensions corrupts replicated messages: original message properties are copied to extensions first (line 181), then addExtension("topic"/"brokerName"/"queueId") overwrites any same-named user property, silently losing source data. Additionally, putExtendMsgProperty writes all extensions back as message properties (connect-ext-topic, connect-ext-brokerName, connect-ext-queueId), so every replicated message gains three properties the original never had. Use a dedicated prefix (e.g. connect-internal-*) or a separate mechanism for routing metadata instead of the generic "topic" key, which also risks hijacking the routing of any other connector whose records happen to carry a "topic" extension. |
||
| connectRecord.addExtension("brokerName",msg.getBrokerName()); | ||
| KeyValue kv = new DefaultKeyValue(); | ||
| kv.put("queueId",msg.getQueueId()); | ||
| connectRecord.addExtension(kv); | ||
|
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. Compatibility: Uses |
||
| res.add(connectRecord); | ||
| } | ||
| break; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,7 +145,9 @@ public static List<KeyValue> groupPartitions(List<String> elements, RmqConnector | |
| assigned++; | ||
| } | ||
| keyValue.put(TaskConfigEnum.TASK_STORE_ROCKETMQ.getKey(), tdc.getStoreTopic()); | ||
| keyValue.put(TaskConfigEnum.TASK_TARGET_ROCKETMQ.getKey(), tdc.getTargetNamesrvs()); | ||
| keyValue.put(TaskConfigEnum.TASK_SOURCE_ROCKETMQ.getKey(), tdc.getSrcNamesrvs()); | ||
| keyValue.put(TaskConfigEnum.TASK_TARGET_CLUSTER.getKey(), tdc.getTargetCluster()); | ||
| keyValue.put(TaskConfigEnum.TASK_SOURCE_CLUSTER.getKey(), tdc.getSrcCluster()); | ||
| keyValue.put(TaskConfigEnum.TASK_OFFSET_SYNC_TOPIC.getKey(), tdc.getOffsetSyncTopic()); | ||
| keyValue.put(TaskConfigEnum.TASK_DATA_TYPE.getKey(), DataType.OFFSET.ordinal()); | ||
|
|
@@ -194,14 +196,30 @@ public static DefaultMQAdminExt startTargetMQAdminTool( | |
| } | ||
|
|
||
| public static DefaultMQAdminExt startMQAdminTool(TaskConfig taskConfig) throws MQClientException { | ||
| RPCHook rpcHook = null; | ||
| if (taskConfig.isSrcAclEnable()) { | ||
| rpcHook = new AclClientRPCHook(new SessionCredentials(taskConfig.getSrcAccessKey(), taskConfig.getSrcSecretKey())); | ||
| } | ||
| DefaultMQAdminExt sourceMQAdminExt = new DefaultMQAdminExt(rpcHook); | ||
| sourceMQAdminExt.setNamesrvAddr(taskConfig.getSourceRocketmq()); | ||
| sourceMQAdminExt.setAdminExtGroup(ConstDefine.REPLICATOR_TASK_ADMIN_GROUP); | ||
| sourceMQAdminExt.setInstanceName(Utils.createUniqInstanceName(taskConfig.getSourceRocketmq())); | ||
|
|
||
| sourceMQAdminExt.start(); | ||
| log.info("Source: RocketMQ sourceMQAdminExt started."); | ||
|
|
||
| return sourceMQAdminExt; | ||
| } | ||
|
|
||
| public static DefaultMQAdminExt startTarMQAdminTool(TaskConfig taskConfig) throws MQClientException { | ||
|
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. startTarMQAdminTool uses source ACL credentials ( |
||
| RPCHook rpcHook = null; | ||
| if (taskConfig.isSrcAclEnable()) { | ||
|
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. startTarMQAdminTool connects to taskConfig.getTargetRocketmq() but authenticates with SOURCE ACL credentials (isSrcAclEnable/getSrcAccessKey/getSrcSecretKey). TaskConfig has no target ACL fields even though RmqConnectorConfig does (see startTargetMQAdminTool using isTargetAclEnable), so with ACL enabled on the target cluster under different credentials every admin call fails. Also this method is a near-duplicate of startMQAdminTool/startTargetMQAdminTool — add target ACL fields to TaskConfig and reuse one implementation. |
||
| rpcHook = new AclClientRPCHook(new SessionCredentials(taskConfig.getSrcAccessKey(), taskConfig.getSrcSecretKey())); | ||
| } | ||
| DefaultMQAdminExt targetMQAdminExt = new DefaultMQAdminExt(rpcHook); | ||
| targetMQAdminExt.setNamesrvAddr(taskConfig.getSourceRocketmq()); | ||
| targetMQAdminExt.setNamesrvAddr(taskConfig.getTargetRocketmq()); | ||
| targetMQAdminExt.setAdminExtGroup(ConstDefine.REPLICATOR_TASK_ADMIN_GROUP); | ||
| targetMQAdminExt.setInstanceName(Utils.createUniqInstanceName(taskConfig.getSourceRocketmq())); | ||
| targetMQAdminExt.setInstanceName(Utils.createUniqInstanceName(taskConfig.getTargetRocketmq())); | ||
|
|
||
| targetMQAdminExt.start(); | ||
| log.info("TARGET: RocketMQ targetMQAdminExt started."); | ||
|
|
||
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.
The meta replicator example sets source-cluster and target-cluster to the same value ("test1-rocketmq"), which is almost certainly a copy-paste mistake and confusing for users setting up cross-cluster replication; also the example still documents offset.sync.topic even though offset sync no longer flows through a topic (direct admin updates now).