-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[Feature][Connector-redis] fix redis cluster bug and add cluster e2e #9869
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: dev
Are you sure you want to change the base?
Conversation
} | ||
} | ||
``` |
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.
I have a question. Text format has been applied to Redis here. However, it is still not supported in HTTP. Do we need to modify the HTTP connector separately in the future? @Hisoka-X
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.
Yes, we can do the same thing in HTTP. LocalFile already did this.
protected String getCustomKey(SeaTunnelRow element, List<String> fields, String keyField) { | ||
Matcher matcher = PLACEHOLDER_PATTERN.matcher(keyField); | ||
StringBuffer result = new StringBuffer(); |
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.
Let's reuse PlaceholderUtils
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.
done, also the placeholder representation in the original configuration needs to be changed, from :{placeholder} to :${placeholder}
List<String> resultKeys; | ||
String nextCursor; | ||
|
||
try (Jedis jedis = new Jedis(pool.getResource())) { |
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.
Can we cache Jedis client to save CPU cost?
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.
done, replace the scanKeyResult
method in the cluster mode with directly using the jedisCluster.scan
method
for (Map.Entry<String, ConnectionPool> entry : nodes.entrySet()) { | ||
ConnectionPool pool = entry.getValue(); | ||
try { | ||
try (Jedis jedis = new Jedis(pool.getResource())) { |
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.
ditto
} | ||
|
||
@Override | ||
public String info() { |
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.
Could you explain why this method can fix #9865 ?
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.
It turns out that when writing to set type data, the batchWriteSet
method of RedisClusterClient
class:RedisDataType'.SET.set(this,...)
. The passed jedis object is this
, but this jedis object actually did not initialize the Connection
connection, which led to the error #9865. The actual jedis object that should be used is the Jedis object passed in from the constructor.
The info
method rewritten in JedisWrapper
is designed to support querying redis version information in cluster mode. The JedisWrapper
class inherits from the Jedis
class, but does not initialize the Connection
object of the parent class. Therefore, the execution of all Jedis-related methods should go through JedisCluster
. Instead of executing the methods in the parent class Jedis (all of which will result in execution errors due to the Connection
not being initialized).
Purpose of this pull request
fix: #9865
The main job of pr:
ReidsClusterClient
, whereRedisDataType.SET.set
passes in the created jedis object instead ofthis
to fix [Bug] [connector-redis] redis cluster mode to write set data error #9865Does this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide