ruanwenjun commented on PR #1857: URL: https://github.com/apache/incubator-seatunnel/pull/1857#issuecomment-1125039699
> Hi @ruanwenjun - > > Thanks for your review and explanation. > > My apology for the confusion. I did not mean to remove checkConfig method (but the line which is [ CheckConfigUtil.checkAllExists(config, HOST, PORT)](https://github.com/apache/incubator-seatunnel/blob/2786a5881d72828b2c8c32ebbc997209284582ee/seatunnel-connectors/seatunnel-connectors-spark/seatunnel-connector-spark-redis/src/main/scala/org/apache/seatunnel/spark/redis/sink/Redis.scala#L58). > > From what I understand, the `HOST` and `PORT` parameters [are optional](https://seatunnel.apache.org/docs/2.1.1/connector/sink/Redis#options). So do we still need the line `CheckConfigUtil.checkAllExists(config, HOST, PORT)` ? If so, should put it in the `if` block ? Here is the link [to the line in PR ](https://github.com/apache/incubator-seatunnel/pull/1857/files#diff-0f9cbcfd02713592b1757e7440c719f3cbcfee6030d83a276c981311e9ffcbbaL59). > > Please let me know if I have mis-understood anything. > > Once again, thanks for your explanation. > > Mans You are right, these two are optional, it's OK to remove this to avoid throw exception. But I suggestion we can set the default value in setConfig method, now we set the default value at prepare method. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
