-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[hotfix][redis] fix npe cause by null host parameter #8881
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
[hotfix][redis] fix npe cause by null host parameter #8881
Conversation
Could you provide the error stack? |
stack info Process finished with exit code 1 |
Hisoka-X
left a comment
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.
Maybe change port from int to Integer is a better option. https://github.com/apache/seatunnel/pull/8881/files#diff-85c72df044b54265682571cdb07752eb396fdf68965cbcf863553efa9a22b34fR49
Okay, thank you for your suggestion. After rounding it to Integer, I can run it locally |
| public class RedisParameters implements Serializable { | ||
| private String host; | ||
| private int port; | ||
| private Integer port; |
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 add some test case for RedisParameters.
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.
How about update it to redis default port 6379.
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.
How about update it to redis default port 6379.
In cluster mode it should be null.
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.
How about update it to redis default port 6379.
In cluster mode it should be null.
In cluster moe, it won't be use
https://github.com/apache/seatunnel/pull/8841/files#diff-85c72df044b54265682571cdb07752eb396fdf68965cbcf863553efa9a22b34fR175
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.
OK, let's update it to 6379 as default. @fcb-xiaobo
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.
Okay, thank you for your suggestions. I will update the code and documentation later


Purpose of this pull request
When the host parameter is not filled in, the config. get method will throw a null pointer exception
Does this PR introduce any user-facing change?
fix #8841
How was this patch tested?
Check list
New License Guide
release-note.