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(cloudevents-server): fix the issue of the EventConsumer close calls #207

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

wuhuizuo
Copy link
Contributor

@wuhuizuo wuhuizuo commented Nov 18, 2024

wrong added the defer close call, it will caused the consumer reading failure.

I tested the change manually, it's passed.

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

wrong added the defer close call.

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

ti-chi-bot bot commented Nov 18, 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 title and description, it looks like the changes made were to fix an issue with the EventConsumer close calls. The main change is the removal of the defer ec.Close() call from the Start method, which was causing the consumer to be closed prematurely.

As for potential problems, it's hard to say for sure without more context on the codebase and how this change fits into it. However, it's worth noting that removing a defer call could have unintended consequences if there are other parts of the codebase that rely on that call being made. Additionally, it's unclear why the log.Info() call was removed from the Start method.

As for suggestions, it would be helpful to have more information on the issue that was being fixed. If possible, it would be good to have some test cases to ensure that the fix does indeed solve the problem. Additionally, if the log.Info() call was removed intentionally, it would be good to document why that call was unnecessary or problematic.

@ti-chi-bot ti-chi-bot bot added the size/XS label Nov 18, 2024
@wuhuizuo
Copy link
Contributor Author

/review default

Copy link

ti-chi-bot bot commented Nov 18, 2024

@wuhuizuo:

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

Based on the pull request title and description, the key changes include fixing an issue with the EventConsumer close calls in the cloudevents-server/pkg/events/handler/kafka.go file. The change involves removing the defer ec.Close() call from the Start function and moving it to the Close function.

The potential problem with this change is that it might cause issues with the consumer reading if the Start function exits early and the Close function is not called. Additionally, the log.Info() call in the Start function has been removed, which might reduce the visibility of the consumer starting up.

To fix these issues, it might be helpful to add a defer ec.Close() call to the Start function above the goroutine, which ensures that the Close function is always called even if the Start function exits early. Additionally, it might be helpful to add back the log.Info() call to the Start function to provide better visibility into the consumer starting up. These changes will help ensure that the consumer is correctly closed and that any potential issues are more easily visible.

In response to this:

/review default

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Nov 18, 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 Nov 18, 2024
@ti-chi-bot ti-chi-bot bot merged commit fe46963 into main Nov 18, 2024
7 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/kafka-read-eof-error branch November 18, 2024 10:26
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