[WIP][ISSUE #22]rocketmq-connect-redis adapt to the new connect api - #31
[WIP][ISSUE #22]rocketmq-connect-redis adapt to the new connect api#31doubleDimple wants to merge 5 commits into
Conversation
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
This PR modifies 23 file(s) with 752 lines of diff. Changes look reasonable.
Automated review by github-manager-bot
| @@ -1,4 +1,20 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
Large diff (752 lines). Consider breaking into smaller, focused PRs for easier review.
|
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 support_redis_connent
git rebase origin/develop
# resolve conflicts, then:
git push --force-with-leaseThis 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 PR has been marked as [WIP] since March 2022 (over 4 years) and currently has merge conflicts. Status check: Is the Redis connector adaptation to the new Connect API still being worked on? The implementation looks substantial (source/sink connectors, event handlers, processors, and comprehensive tests), but needs to be rebased. If this is still relevant, please update the status and rebase. If the work has been abandoned or superseded by another effort, consider closing. Automated review by github-manager-bot |
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
PR received and logged for review. This PR requires detailed code review by a maintainer.
Diff size: 752 lines
Author: doubleDimple (CONTRIBUTOR)
Automated review by RockteMQ-AI
|
|
||
|
|
||
| public Boolean push(Map<Field, Object[]> fieldMap, EntryType entryType) { | ||
|
|
There was a problem hiding this comment.
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.
| Object[] value = JSONObject.parseArray((String)fieldValue).toArray(); | ||
| if (value.length == 2) { | ||
| fieldMap.put(field, value); | ||
| } else { |
There was a problem hiding this comment.
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.
| List<Field> fields = schema.getFields(); | ||
| Boolean parseError = false; | ||
| if (!fields.isEmpty()) { | ||
| for (Field field : fields) { |
There was a problem hiding this comment.
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.
| Schema schema = sinkDataEntry.getSchema(); | ||
| EntryType entryType = sinkDataEntry.getEntryType(); | ||
|
|
||
| List<Field> fields = schema.getFields(); |
There was a problem hiding this comment.
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.
| @Override | ||
| public void start(KeyValue keyValue) { | ||
| this.kvEntryConverter = new RedisEntryConverter(); | ||
|
|
There was a problem hiding this comment.
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.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| return config; | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
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.
|
|
||
| @Override | ||
| public String verifyAndSetConfig(KeyValue config) { | ||
| this.keyValue = config; |
There was a problem hiding this comment.
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.
| this.eventProcessor = eventProcessor; | ||
| } | ||
|
|
||
| public Config getConfig() { |
There was a problem hiding this comment.
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.
| @@ -0,0 +1,15 @@ | |||
| package org.apache.rocketmq.connect.redis.sink; | |||
There was a problem hiding this comment.
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.
3 participants
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.[ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyleto make sure basic checks pass. Runmvn clean install -DskipITsto make sure unit-test pass. Runmvn clean test-compile failsafe:integration-testto make sure integration-test pass.