Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions connectors/rocketmq-connect-redis/pom.xml
Original file line number Diff line number Diff line change
@@ -1,4 +1,20 @@
<?xml version="1.0" encoding="UTF-8"?>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Large diff (752 lines). Consider breaking into smaller, focused PRs for easier review.

<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
Expand Down Expand Up @@ -107,6 +123,17 @@
<encoding>${project.build.sourceEncoding}</encoding>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.rat</groupId>
<artifactId>apache-rat-plugin</artifactId>
<version>0.12</version>
<configuration>
<excludes>
<exclude>README.md</exclude>
<exclude>README-CN.md</exclude>
</excludes>
</configuration>
</plugin>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>findbugs-maven-plugin</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
* limitations under the License.
*/

package org.apache.rocketmq.connect.redis.common;
package org.apache.rocketmq.connect.redis.config;

import java.net.URISyntaxException;
import java.nio.ByteBuffer;
Expand All @@ -26,6 +26,8 @@

import com.moilioncircle.redis.replicator.RedisURI;
import org.apache.commons.lang.StringUtils;
import org.apache.rocketmq.connect.redis.common.RedisConstants;
import org.apache.rocketmq.connect.redis.common.SyncMod;
import org.apache.rocketmq.connect.redis.util.PropertyToObjectUtils;
import io.openmessaging.KeyValue;
import org.slf4j.Logger;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.rocketmq.connect.redis.connector;

import io.openmessaging.KeyValue;
import io.openmessaging.connector.api.Task;
import io.openmessaging.connector.api.sink.SinkConnector;
import org.apache.rocketmq.connect.redis.config.Config;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.ArrayList;
import java.util.List;

/**
* author: doubleDimple
*/
public class RedisSinkConnector extends SinkConnector {

private static final Logger LOGGER = LoggerFactory.getLogger(RedisSinkConnector.class);

private volatile boolean configValid = false;
private volatile boolean adminStarted;
private KeyValue keyValue;

@Override

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

configValid is set to true in verifyAndSetConfig but never read anywhere. adminStarted is declared but never used. These are dead fields that add confusion.

public String verifyAndSetConfig(KeyValue config) {
this.keyValue = config;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

taskConfigs() always returns a single-element list containing the full config regardless of the requested task count. The framework may call this with a parallelism hint; the connector ignores it and cannot scale to multiple tasks.

String msg = Config.checkConfig(keyValue);
if (msg != null) {
return msg;
}
this.configValid = true;
return null;
}

@Override
public void start() {
LOGGER.info("the redisSinkConnector is start...");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

start() only logs a message and performs no initialization (no connection test, no admin client setup). A connector start() should validate connectivity or at least initialize shared resources.


@Override
public void stop() {

}

@Override
public void pause() {

}

@Override
public void resume() {

}

@Override
public Class<? extends Task> taskClass() {
return RedisSinkTask.class;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

taskConfigs() always returns exactly one config. For multi-partition or scaled deployments, this prevents parallelism. The connector should accept a maxTasks parameter and distribute configs accordingly.

}

@Override
public List<KeyValue> taskConfigs() {
List<KeyValue> keyValues = new ArrayList<>();
keyValues.add(this.keyValue);
return keyValues;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.rocketmq.connect.redis.connector;

import com.alibaba.fastjson.JSONObject;
import io.openmessaging.KeyValue;
import io.openmessaging.connector.api.common.QueueMetaData;
import io.openmessaging.connector.api.data.EntryType;
import io.openmessaging.connector.api.data.Field;
import io.openmessaging.connector.api.data.Schema;
import io.openmessaging.connector.api.data.SinkDataEntry;
import io.openmessaging.connector.api.sink.SinkTask;
import org.apache.rocketmq.connect.redis.config.Config;
import org.apache.rocketmq.connect.redis.converter.KVEntryConverter;
import org.apache.rocketmq.connect.redis.converter.RedisEntryConverter;
import org.apache.rocketmq.connect.redis.handler.DefaultRedisEventHandler;
import org.apache.rocketmq.connect.redis.handler.RedisEventHandler;
import org.apache.rocketmq.connect.redis.processor.DefaultRedisEventProcessor;
import org.apache.rocketmq.connect.redis.processor.RedisEventProcessor;
import org.apache.rocketmq.connect.redis.sink.RedisUpdater;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

/**
* author doubleDimple
*/
public class RedisSinkTask extends SinkTask {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

updater is never initialized — updater.push(...) on line 93 will always throw a NullPointerException. The RedisUpdater is a stub that returns null anyway, making the entire sink path non-functional.

private static final Logger LOGGER = LoggerFactory.getLogger(RedisSinkTask.class);

private RedisUpdater updater;

/**
* listening and handle Redis event.
*/
private RedisEventProcessor eventProcessor;
private Config config;
/**
* convert kVEntry to list of sourceDataEntry
*/
private KVEntryConverter kvEntryConverter;

public RedisEventProcessor getEventProcessor() {
return eventProcessor;
}

public void setEventProcessor(RedisEventProcessor eventProcessor) {
this.eventProcessor = eventProcessor;
}

public Config getConfig() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

commit() is a no-op. For at-least-once delivery guarantees the framework relies on this callback to advance committed offsets. Leaving it empty means offsets are never acknowledged, which may cause the framework to redeliver all messages on restart.

return config;
}

@Override

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 eventProcessor field and its associated RedisEventHandler are started in start(), but the sink task's put() method never reads from the processor — it only writes to Redis via updater. Starting a full replication event processor (which connects to Redis as a replica) in a sink task is architecturally wrong and will create a redundant Redis replication stream.

public void put(Collection<SinkDataEntry> sinkDataEntries) {
//save data from MQ to redis
for (SinkDataEntry sinkDataEntry : sinkDataEntries) {
Map<Field, Object[]> fieldMap = new HashMap<>();
Object[] payloads = sinkDataEntry.getPayload();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

kvEntryConverter is initialized in start() but never used in put() or anywhere else in this class. It appears to be leftover dead code.


Schema schema = sinkDataEntry.getSchema();
EntryType entryType = sinkDataEntry.getEntryType();

List<Field> fields = schema.getFields();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Boolean parseError is declared with boxed Boolean (object type) instead of primitive boolean. While initialized to false, using the boxed type is unnecessary here and could theoretically cause a NullPointerException if the variable were ever left uninitialized in a refactored code path.

Boolean parseError = false;
if (!fields.isEmpty()) {
for (Field field : fields) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

updater field is never initialized — it is declared but no assignment appears in start() or anywhere else. Every call to put() will throw a NullPointerException when updater.push(...) is invoked.

Object fieldValue = payloads[field.getIndex()];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

JSONObject.parseArray((String)fieldValue) will throw an unchecked exception if fieldValue is null or not valid JSON. No null-check or try-catch protects this call, so a single malformed record will crash the entire put() batch.

Object[] value = JSONObject.parseArray((String)fieldValue).toArray();
if (value.length == 2) {
fieldMap.put(field, value);
} else {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unsafe cast: (String) fieldValue will throw ClassCastException if the payload value is not a String. There is no type check or null guard before the cast and the subsequent JSONObject.parseArray() call.

LOGGER.error("parseArray error, fieldValue:{}", fieldValue);
parseError = true;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When updater.push() fails or parseError is true, the record is silently dropped after logging. There is no dead-letter routing, retry, or error-reporting mechanism, which risks silent data loss in production.

}
}
if (!parseError) {
Boolean isSuccess = updater.push(fieldMap, entryType);
if (!isSuccess) {
LOGGER.error("push data error, entryType:{}, fieldMap:{}", fieldMap, entryType);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

commit() is empty — there is no offset tracking or acknowledgment logic. If the framework relies on the sink task to confirm processed offsets, this could lead to duplicate processing on restart.

}
}
}

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 kvEntryConverter field is initialized in start() but never used anywhere in the class. The sink path bypasses the converter and directly parses JSON in put(), making the converter dead code and leaving the abstraction incomplete.

@Override
public void commit(Map<QueueMetaData, Long> offsets) {

}

@Override

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 sink task's start() creates a DefaultRedisEventProcessor and DefaultRedisEventHandler — these are source-side replication components (they listen to Redis replication stream). This is architecturally wrong for a sink task that should be writing data into Redis.

public void start(KeyValue keyValue) {
this.kvEntryConverter = new RedisEntryConverter();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

e.printStackTrace() is called in start() before the structured log statement. This sends the stack trace to stderr outside the logging framework, making it invisible in log aggregators. Use only LOGGER.error(...) with the exception as the last argument.

this.config = new Config();
this.config.load(keyValue);
LOGGER.info("task config msg: {}", this.config.toString());

this.eventProcessor = new DefaultRedisEventProcessor(config);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

e.printStackTrace() is used instead of logging through SLF4J. This bypasses the configured logging infrastructure and is inappropriate for production connector code.

RedisEventHandler eventHandler = new DefaultRedisEventHandler(this.config);
this.eventProcessor.registEventHandler(eventHandler);
try {
this.eventProcessor.start();
LOGGER.info("Redis task start.");
} catch (IOException e) {
e.printStackTrace();
LOGGER.error("processor start error: [{}]", e.getMessage());
this.stop();
}
}

@Override
public void stop() {
if (this.eventProcessor != null) {
try {
this.eventProcessor.stop();
LOGGER.info("Redis task is stopped.");
} catch (IOException e) {
LOGGER.error("processor stop error: {}", e);
}
}
}

@Override
public void pause() {

}

@Override
public void resume() {

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import io.openmessaging.KeyValue;
import io.openmessaging.connector.api.Task;
import io.openmessaging.connector.api.source.SourceConnector;
import org.apache.rocketmq.connect.redis.common.Config;
import org.apache.rocketmq.connect.redis.config.Config;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import io.openmessaging.KeyValue;
import io.openmessaging.connector.api.data.SourceDataEntry;
import io.openmessaging.connector.api.source.SourceTask;
import org.apache.rocketmq.connect.redis.common.Config;
import org.apache.rocketmq.connect.redis.config.Config;
import org.apache.rocketmq.connect.redis.common.Options;
import org.apache.rocketmq.connect.redis.converter.KVEntryConverter;
import org.apache.rocketmq.connect.redis.converter.RedisEntryConverter;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import com.moilioncircle.redis.replicator.event.Event;
import com.moilioncircle.redis.replicator.rdb.datatype.KeyValuePair;
import com.moilioncircle.redis.replicator.rdb.iterable.datatype.BatchedKeyValuePair;
import org.apache.rocketmq.connect.redis.common.Config;
import org.apache.rocketmq.connect.redis.config.Config;
import org.apache.rocketmq.connect.redis.common.SyncMod;
import org.apache.rocketmq.connect.redis.parser.DefaultRedisRdbParser;
import org.apache.rocketmq.connect.redis.parser.RedisRdbParser;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
import com.moilioncircle.redis.replicator.Replicator;
import com.moilioncircle.redis.replicator.event.EventListener;
import org.apache.commons.lang.StringUtils;
import org.apache.rocketmq.connect.redis.common.Config;
import org.apache.rocketmq.connect.redis.config.Config;
import org.apache.rocketmq.connect.redis.common.RedisConstants;
import org.apache.rocketmq.connect.redis.common.SyncMod;
import org.apache.rocketmq.connect.redis.handler.RedisEventHandler;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import com.moilioncircle.redis.replicator.event.PreRdbSyncEvent;
import com.moilioncircle.redis.replicator.rdb.datatype.AuxField;
import java.io.IOException;
import org.apache.rocketmq.connect.redis.common.Config;
import org.apache.rocketmq.connect.redis.config.Config;
import org.apache.rocketmq.connect.redis.pojo.RedisEvent;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package org.apache.rocketmq.connect.redis.sink;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing Apache License header. All other new/modified files in this PR have the ASF license header added, but RedisUpdater.java does not, which will fail the apache-rat license check added in pom.xml.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing Apache License header. All other new files in this PR include the standard ASF license block. This will also cause apache-rat:check to fail.


import io.openmessaging.connector.api.data.EntryType;
import io.openmessaging.connector.api.data.Field;

import java.util.Map;

public class RedisUpdater {


public Boolean push(Map<Field, Object[]> fieldMap, EntryType entryType) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

RedisUpdater.push() is a stub that always returns null. RedisSinkTask.put() calls this method and checks the Boolean return value without null-safety, which will cause a NullPointerException at if (!isSuccess) on every invocation. The sink task is completely non-functional.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

push() always returns null (a stub). This makes the entire sink data path a no-op. The PR is marked WIP but this class should at minimum throw UnsupportedOperationException or be clearly documented as unimplemented to avoid silent data loss.

return null;
}
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,20 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.rocketmq.redis.test.common;

import java.nio.ByteBuffer;
Expand All @@ -6,7 +23,7 @@
import com.moilioncircle.redis.replicator.RedisURI;
import io.openmessaging.KeyValue;
import io.openmessaging.internal.DefaultKeyValue;
import org.apache.rocketmq.connect.redis.common.Config;
import org.apache.rocketmq.connect.redis.config.Config;
import org.apache.rocketmq.connect.redis.common.SyncMod;
import org.junit.Assert;
import org.junit.Test;
Expand Down
Loading