Skip to content
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

fix(publisher): fix redis command usage #184

Merged
merged 2 commits into from
Oct 22, 2024
Merged

Conversation

wuhuizuo
Copy link
Contributor

@wuhuizuo wuhuizuo commented Oct 21, 2024

This pull request introduces Redis integration into the publisher service, refactoring the code to accommodate the new Redis client and updating the handling of Redis commands. The most important changes include adding the Redis client configuration, modifying the Publisher and Service structs, and updating Redis command methods.

Redis Integration:

Struct Modifications:

  • publisher/pkg/config/config.go: Added a Redis struct to hold Redis configuration and updated the Worker and Service structs to include this new Redis field.

Redis Command Updates:

Signed-off-by: wuhuizuo wuhuizuo@126.com

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Copy link

ti-chi-bot bot commented Oct 21, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Based on the pull request, the key changes involve the modification of the redisClient variable in two different files. The redisClient variable was changed from *redis.Client to redis.Cmdable. The redisClient.Set() function calls were also changed to redisClient.SetXX() and redisClient.SetNX() in different parts of the code.

There are no potential problems that can be identified in this pull request, and the changes seem to be well-implemented.

However, it is suggested to provide more context in the pull request description, such as the reason for the changes, the impact on the overall codebase, and any testing that has been done to ensure the changes are correct. This will help the reviewers understand the changes better and provide more meaningful feedback.

Additionally, it is recommended to follow consistent naming conventions across the codebase. For example, if the variable name is redisClient in one file, it should be named the same way in all other files where it is being used.

@ti-chi-bot ti-chi-bot bot requested a review from purelind October 21, 2024 09:01
@ti-chi-bot ti-chi-bot bot added the size/S label Oct 21, 2024
Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Copy link

ti-chi-bot bot commented Oct 22, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

This pull request aims to fix the Redis command usage in the publisher application. Specifically, it adds a Redis struct to the config package, modifies the NewPublisher function and Publisher struct in the tiup package to accept a redisClient, and uses redisClient to replace redis.Client() in the Publisher.Handle function.

There are no potential problems with this pull request. However, it would be better to use SetNX instead of SetXX in the tiupsrvc.RequestToPublish function to avoid accidentally overwriting existing values.

Here is the suggested fix:

-		if err := s.redisClient.SetXX(ctx, requestID, PublishStateQueued, s.stateTTL).Err(); err != nil {
+		if err := s.redisClient.SetNX(ctx, requestID, PublishStateQueued, s.stateTTL).Err(); err != nil {

Overall, the changes look good and should be merged.

@ti-chi-bot ti-chi-bot bot added size/M and removed size/S labels Oct 22, 2024
@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Oct 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the approved label Oct 22, 2024
@ti-chi-bot ti-chi-bot bot merged commit a7470b6 into main Oct 22, 2024
3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/publisher-redis-usage branch October 22, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant