feat: Support SubscribeTopicEvents API #1078
Conversation
Signed-off-by: “huazhongming” <[email protected]>
WalkthroughThe changes in this pull request encompass multiple files, focusing on enhancing the functionality of a Pub/Sub system. Key modifications include the introduction of a subscription server, new methods for subscribing to topics, improved error handling in existing methods, and the addition of new message types and RPC methods in the protocol definitions. Documentation has also been updated for clarity, particularly regarding the running of demos. Changes
Poem
Original articleOriginal articleOriginalOriginal
Original articleOriginal articleThank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? ❤️ Share
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| File Path | Change Summary |
|---|---|
| demo/pubsub/README.md | Updated instructions for running the demo; added details about the new "appcallback" component. |
| pkg/grpc/default_api/api.go | Added fields for concurrency management in the api struct, including closeCh and wg. |
| pkg/grpc/default_api/api_pubsub.go | Refactored publishMessageGRPC to call a new helper method, envelopeFromSubscriptionMessage, for message processing. |
| pkg/grpc/default_api/api_subscribe.go | Implemented gRPC API for subscribing to topic events; defined structures for managing subscriptions and connections. |
| pkg/grpc/default_api/api_subscribe_test.go | Introduced unit tests for subscription and publishing functionalities, simulating various scenarios. |
| spec/proto/runtime/v1/runtime.proto | Added new RPC method SubscribeTopicEvents and several message types to support subscription functionality. |
Poem
In the meadow where bunnies play,
New features hop in, brightening the day.
With subscriptions and messages that flow,
The code now dances, putting on a show!
So let us cheer, with a twitch of our nose,
For changes that bloom like a fresh garden rose! 🐇🌼
[! WARNING]
Rate limit exceeded
@CrazyHZM has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 0 seconds before requesting another review.
⌛ How to resolve this issue?
After the wait time has elapsed, a review can be triggered using the
@coderabbitai reviewcommand as a PR comment. Alternatively, push new commits to this PR.We recommend that you space out your commits to avoid hitting the rate limit.
🚦 How do rate limits work?
CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans.In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.
Original article
Original article
原文
原文
[!WARNING]
Rate limit exceeded
@CrazyHZM has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 0 seconds before requesting another review.
⌛ How to resolve this issue?
After the wait time has elapsed, a review can be triggered using the
@coderabbitai reviewcommand as a PR comment. Alternatively, push new commits to this PR.We recommend that you space out your commits to avoid hitting the rate limit.
🚦 How do rate limits work?
CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.
Original article
Original article
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
❤️ Share
- X
- Mastodon
- [Reddit]( https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.% 20Check%20it%20out%3A%20https%3A//coderabbit.ai)
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate thehigh-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit. ai/integrations/schema.v2.json
Documentation and Community
- Visit our [Docu
Original
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
❤️ Share
- X
- Mastodon
- [LinkedIn]( https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit. ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic% 21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiina new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pausethe reviewson a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit. ai/integrations/schema.v2.json
Documentation and Community
- Visit our [Docu
Original
Original article
Original article
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
❤️ Share
- X
- [Mastodon]( https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for% 20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai)
🪧 Tips
Chat
There are 3ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in thePR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit. ai/integrations/schema.v2.json
Documentation and Community
- Visit our Documentation for detailed information on how to use CodeRabbit.
- Join our Discord Community to get help, request featur
Original
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
❤️ Share
- X
- [Mastodon]( https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F% 2Fcoderabbit.ai)
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the filesin the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in thePR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit. ai/integrations/schema.v2.json
Documentation and Community
- Visit our Documentation for detailed information on how to use CodeRabbit.
- Join our Discord Community to get help, request featur
Original
Original article
Original article
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
❤️ Share
- X
- Mastodon
- [Reddit]( https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s% 20free%20for% 20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai)
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR descriptionto prevent this PRfrom being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit. ai/integrations/schema.v2.json
Documentation and Community
- Visit our [Docu
Original
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow.If you find it useful,would you consider giving us a shout-out on your favorite social media?
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.-@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbitConfiguration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit. ai/integrations/schema.v2.json
Documentation and Community
- Visit our [Docu
Original
Original article
Original article
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix incommit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.-@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitaihelpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit. ai/integrations/schema.v2.json
Documentation and Community
- Visit our Documentation for detailed information on how to us
Original
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix incommit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitaihelpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit. ai/integrations/schema.v2.json
Documentation and Community
- Visit our Documentation for detailed information on how to us
Original
Original article
Original article
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
❤️ Share
- X
- Mastodon
- [Reddit]( https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.% 20Check%20it%20out%3A%20https%3A//coderabbit.ai)
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focuseddiscussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit. ai/integrations/schema.v2.json
Documentation and Community
- Visit our [Docu
Original
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
❤️ Share
- [X]( https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon]( https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit]( https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn]( https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit. ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic% 21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PRbranch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviewson a PR.@coderabbitai resumetoresume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to theroot of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit. ai/integrations/schema.v2.json
Documentation and Community
- Visit our [Docu
Original
Original article
Original article
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
❤️ Share
- X
- [Mastodon]( https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A% 20https%3A%2F%2Fcoderabbit.ai)
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.-@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in thePR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit. ai/integrations/schema.v2.json
Documentation and Community
- Visit our Documentation for detailed information on how to use CodeRabbit.
- Join our Discord Community to get help, request featur
Original
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit.Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration forthe repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in thePR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit. ai/integrations/schema.v2.json
Documentation and Community
- Visit our Documentation fordetailed information on how to use CodeRabbit.
- Join our Discord Community to get help, request featur
Original
Original article
Original article
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
❤️ Share
- X
- Mastodon
- [Reddit]( https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for% 20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai)
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit. ai/integrations/schema.v2.json
Documentation and Community
- Visit our [Docu
Original
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit. ai/integrations/schema.v2.json
Documentation and Community
- Visit our [Docu
Original
Original article
Original
[!WARNING]
There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.🔧 golangci-lint
level=warning msg="[config_reader] The configuration option
run.skip-filesis deprecated, please useissues.exclude-files."
level=warning msg="[config_reader] The configuration optionrun.skip-dirsis deprecated, please useissues.exclude-dirs."
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.
CodeRabbit Commands (Invoked using PR comments)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
Documentation and Community
- Visit our Documentation for detailed information on how to use CodeRabbit.
- Join our Discord Community to get help, request features, and share feedback.
- Follow us on X/Twitter for updates and announcements.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (15)
demo/pubsub/README.md (2)
Line range hint
5-35: Define the ${project_path} variable and fix command formatting.The instructions need some improvements for better clarity and correctness:
- The
${project_path}variable is used but not defined. Add a note explaining how to set it:+# Set the project path to your Layotto repository root +export project_path=/path/to/layotto + #################### Run pubsub demo with appcallback ####################
- Fix the subscriber command typo and formatting:
-/.subscriber -s pub_subs_demo +./subscriber -s pub_subs_demo
- Remove extra spaces in publisher commands:
-cd ${project_path}/demo/pubsub/client/ -go build -o publisher - ./publisher -s pub_subs_demo +cd ${project_path}/demo/pubsub/client/ +go build -o publisher +./publisher -s pub_subs_demo
Line range hint
5-35: Enhance documentation with additional details.Consider adding:
- Expected output or success criteria for each demo
- Prerequisites section (required Go version, dependencies)
- Troubleshooting section for common issues
- Brief explanation of the difference between
appcallbackandSubscribeTopicEventsapproachesWould you like me to help generate these additional documentation sections?
demo/pubsub/server/dynamic/subscribe_server.go (4)
31-35: Consider encapsulating configuration in a structGlobal variables make testing difficult and create implicit dependencies. Consider refactoring the configuration into a struct for better maintainability and testability.
-var storeName string +type Config struct { + StoreName string +} + +func NewConfig() *Config { + return &Config{} +} func init() { - flag.StringVar(&storeName, "s", "", "set `storeName`") + config := NewConfig() + flag.StringVar(&config.StoreName, "s", "", "set `storeName`") }
80-80: Replace hard-coded sleep with configurable durationHard-coded sleep durations make testing difficult and may not be suitable for all environments.
- time.Sleep(time.Second * 10) + // Consider using a context with timeout or making the duration configurable + ctx, cancel := context.WithTimeout(context.Background(), time.Second * 10) + defer cancel() + <-ctx.Done()
45-90: Consider splitting testDynamicSub into smaller functionsThe function is doing too many things: client creation, two different types of subscriptions, message handling, and cleanup. This makes it harder to test and maintain.
Consider splitting into smaller functions like:
setupClient()subscribeWithHandler(cli client.Client)subscribeAndReceive(cli client.Client)cleanup(cli client.Client, stop func(), sub client.Subscription)🧰 Tools
🪛 golangci-lint
46-46: ineffectual assignment to err
(ineffassign)
49-49: ineffectual assignment to err
(ineffassign)
92-96: Add input validation and error handling to eventHandlerThe function should validate input and handle potential logging errors.
func eventHandler(request *runtimev1pb.TopicEventRequest) client.SubscriptionResponseStatus { + if request == nil { + log.Printf("Error: received nil request") + return client.SubscriptionResponseStatusRetry + } + log.Printf(">>[SubscribeWithHandler API] Received message\n") log.Printf("event - PubsubName: %s, Topic: %s, ID: %s, Data: %s\n", request.PubsubName, request.Topic, request.Id, request.Data) return client.SubscriptionResponseStatusSuccess }pkg/grpc/default_api/api.go (1)
Line range hint
90-157: Review architecture for subscription handlingThe changes introduce infrastructure for handling subscriptions through streaming and graceful shutdown mechanisms. Consider the following architectural points:
- Ensure proper cleanup of resources in case of errors during subscription setup
- Document the interaction between the streamer and existing pubsub components
- Consider adding metrics/monitoring for subscription health
Would you like assistance in designing these improvements?
pkg/grpc/default_api/api_pubsub.go (1)
156-156: Consider implementing tracing.The TODO comment for tracing remains unaddressed. Tracing would be valuable for debugging and monitoring pub/sub message flow.
Would you like me to help implement tracing using OpenTelemetry or a similar framework?
sdk/go-sdk/client/subscribe.go (1)
22-35: Add comments to exported types and functionsThe exported types from lines 22 to 35 lack comments. In Go, it's best practice to document exported identifiers for better code readability and documentation generation.
Apply this diff to add comments:
+// SubscriptionResponseStatus represents the status of a subscription response. type SubscriptionResponseStatus string +// SubscriptionResponse contains the status of the subscription processing. type SubscriptionResponse struct { Status SubscriptionResponseStatus `json:"status"` } +// SubscriptionHandleFunction defines the handler function for processing subscription events. type SubscriptionHandleFunction func(request *runtimev1pb.TopicEventRequest) SubscriptionResponseStatus +// SubscriptionRequest represents a request to subscribe to a topic in a pub/sub system. type SubscriptionRequest struct { PubsubName string Topic string DeadLetterTopic *string Metadata map[string]string }pkg/grpc/default_api/api_subscribe.go (5)
227-231: Consider adding a timeout when waiting for client responseWaiting indefinitely for a response from the client can lead to resource leaks if the client becomes unresponsive. Introducing a timeout can help prevent the server from blocking indefinitely.
Here's how you might implement a timeout:
+import "time" ... var resp *runtimev1pb.SubscribeTopicEventsRequestProcessed +timeout := time.After(30 * time.Second) select { case <-ctx.Done(): return ctx.Err() +case <-timeout: + return errors.New("timeout waiting for client response") case resp = <-ch: }
192-195: Clarify error message for unexpected client requestsWhen an unexpected message is received from the client, the error message "duplicate initial request received" might be misleading. Consider refining the error message to accurately reflect the issue.
Apply this diff to improve the error message:
if eventResp == nil { - return errors.New("duplicate initial request received") + return errors.New("expected event processing response, but received nil") }
265-267: Avoid potential key collisions in StreamerKeyThe
StreamerKeyfunction concatenatespubsubandtopicusing delimiters. Ifpubsubortopiccontains these delimiters, it could lead to key collisions.Consider using a struct or a safer delimiter to prevent collisions:
-func (s *streamer) StreamerKey(pubsub, topic string) string { - return "___" + pubsub + "||" + topic +func (s *streamer) StreamerKey(pubsub, topic string) string { + return fmt.Sprintf("%q|%q", pubsub, topic) }
73-78: Handle stream reception errors gracefullyWhen receiving the initial request from the stream, if an error occurs, it might be helpful to log the error for debugging purposes before returning.
Apply this diff to log the error:
if err != nil { + log.DefaultLogger.Errorf("failed to receive initial request: %v", err) return err }
210-211: Improve error message when subscriber not foundWhen no subscriber is found for the given
pubsubNameandtopic, the error message could include more context to aid in debugging.Apply this diff to enhance the error message:
if !ok { - return fmt.Errorf("no streamer subscribed to pubsub %q topic %q", pubsubName, msg.Topic) + return fmt.Errorf("no subscriber found for pubsub %q and topic %q", pubsubName, msg.Topic) }spec/proto/runtime/v1/runtime.proto (1)
815-816: Empty message 'SubscribeTopicEventsResponseInitial' might be unnecessaryThe message
SubscribeTopicEventsResponseInitialis defined but contains no fields.If no data is needed in the initial response, consider using
google.protobuf.Emptyinstead to avoid defining an empty message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (5)
pkg/mock/runtime/runtime.pb.gois excluded by!**/*.pb.gospec/proto/extension/v1/s3/oss.pb.gois excluded by!**/*.pb.gospec/proto/extension/v1/s3/oss_grpc.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
components/pluggable/grpc_mock.go(0 hunks)components/sequencer/snowflake/snowflake.go(1 hunks)components/sequencer/snowflake/snowflake_sequencer.go(0 hunks)components/sequencer/snowflake/snowflake_sequencer_test.go(0 hunks)components/sequencer/snowflake/snowflake_test.go(0 hunks)demo/pubsub/README.md(2 hunks)demo/pubsub/server/dynamic/subscribe_server.go(1 hunks)pkg/grpc/default_api/api.go(3 hunks)pkg/grpc/default_api/api_pubsub.go(2 hunks)pkg/grpc/default_api/api_subscribe.go(1 hunks)pkg/grpc/default_api/api_subscribe_test.go(1 hunks)pkg/mock/components/lock/lock.go(1 hunks)sdk/go-sdk/client/client.go(1 hunks)sdk/go-sdk/client/subscribe.go(1 hunks)spec/proto/runtime/v1/runtime.proto(3 hunks)
💤 Files with no reviewable changes (4)
- components/pluggable/grpc_mock.go
- components/sequencer/snowflake/snowflake_sequencer.go
- components/sequencer/snowflake/snowflake_sequencer_test.go
- components/sequencer/snowflake/snowflake_test.go
✅ Files skipped from review due to trivial changes (1)
- components/sequencer/snowflake/snowflake.go
🧰 Additional context used
🪛 golangci-lint
demo/pubsub/server/dynamic/subscribe_server.go
46-46: ineffectual assignment to err
(ineffassign)
49-49: ineffectual assignment to err
(ineffassign)
🪛 buf
spec/proto/runtime/v1/runtime.proto
7-7: import "appcallback.proto": file does not exist
(COMPILE)
🔇 Additional comments (13)
pkg/mock/components/lock/lock.go (2)
12-12: LGTM: Required import for mock implementation
The added import for the lock package is necessary as it provides the types and interfaces that this mock implements.
Line range hint 1-108: Verify mock implementation matches interface definition
Since this is a generated mock file, let's verify that it correctly implements all methods of the LockStore interface.
✅ Verification successful
After comparing the mock implementation with the interface definition, I can generate the final response as the verification is complete.
Mock implementation correctly matches the LockStore interface
The generated mock implementation in pkg/mock/components/lock/lock.go perfectly matches the interface definition from components/lock/lock_store.go. All required methods are present with matching signatures:
Init(metadata Metadata) errorFeatures() []FeatureTryLock(ctx context.Context, req *TryLockRequest) (*TryLockResponse, error)Unlock(ctx context.Context, req *UnlockRequest) (*UnlockResponse, error)LockKeepAlive(context.Context, *LockKeepAliveRequest) (*LockKeepAliveResponse, error)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the mock implements all methods of the LockStore interface
# and that there are no discrepancies in method signatures.
# Find and display the LockStore interface definition
ast-grep --pattern 'type LockStore interface {
$$$
}'
# Find any references to LockStore methods that might not be mocked
rg "type LockStore interface" -A 20
Length of output: 1763
pkg/grpc/default_api/api_subscribe_test.go (2)
1-33: LGTM! Header and imports are well-structured.
The file includes appropriate license header and all necessary imports for testing, mocking, and runtime functionality.
70-134:
Address potential test reliability issues.
The test has several areas that could be improved for reliability and completeness:
- The hardcoded sleep duration could cause flaky tests:
- time.Sleep(1 * time.Second)
+ // Use channels or sync.WaitGroup for coordination instead of sleep- Missing error scenarios and timeout handling:
- err = apiForTest.publishMessageForStream(context.Background(), msg, pubsubName)
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ err = apiForTest.publishMessageForStream(ctx, msg, pubsubName)- Consider adding test cases for:
- Network timeouts
- Invalid message format
- Subscriber not found
- Failed message processing
Also, there's a potential race condition when accessing the subscribers map. Consider adding proper synchronization.
pkg/grpc/default_api/api.go (1)
23-23: LGTM: Required import for atomic operations
The addition of sync/atomic import is appropriate for the new atomic.Bool field.
sdk/go-sdk/client/client.go (3)
61-63: LGTM: Well-documented Subscribe method with proper cleanup guidance.
The method signature and documentation clearly indicate the contract and cleanup requirements. The return types align with Go's idiomatic error handling patterns.
65-67: LGTM: Well-documented SubscribeWithHandler method with proper cleanup guidance.
The method provides a convenient way to handle subscription events with a callback pattern. The documentation properly emphasizes the cleanup requirement by calling the returned cancellation function.
61-68: Verify the interface implementation in GRPCClient.
The new methods have been added to the interface correctly. Let's verify their implementation in the GRPCClient struct.
✅ Verification successful
Interface implementation verified successfully
Both Subscribe and SubscribeWithHandler methods are properly implemented in the GRPCClient struct in sdk/go-sdk/client/subscribe.go. The implementations are complete and match the interface signatures:
Subscribecreates and returns a newSubscriptioninstanceSubscribeWithHandlerbuilds uponSubscribeand handles messages asynchronously with proper error handling and cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that GRPCClient implements the new methods
# Check for the implementation of Subscribe method
ast-grep --pattern 'func (c *GRPCClient) Subscribe(ctx context.Context, request SubscriptionRequest) (*Subscription, error)'
# Check for the implementation of SubscribeWithHandler method
ast-grep --pattern 'func (c *GRPCClient) SubscribeWithHandler(ctx context.Context, request SubscriptionRequest, handler SubscriptionHandleFunction) (func() error, error)'
# Verify that GRPCClient implements all interface methods
rg "type GRPCClient struct" -A 5 -B 5
Length of output: 3581
pkg/grpc/default_api/api_pubsub.go (1)
156-161: LGTM! Clean refactoring of publishMessageGRPC.
The extraction of message processing logic into envelopeFromSubscriptionMessage improves code organization and maintainability.
pkg/grpc/default_api/api_subscribe.go (2)
46-69: Ensure proper synchronization when accessing shared resources
In SubscribeTopicEvents, you are adding to a.wg and launching goroutines that might access shared resources. Ensure that all shared variables are properly synchronized to prevent race conditions.
Run the following script to check for race conditions:
✅ Verification successful
Let me gather more information about the code to verify the synchronization concerns.
Based on the code analysis, I can now provide a final response about the synchronization concerns.
Shared resources are properly synchronized
The implementation shows proper synchronization patterns:
- The
WaitGroupusage is correctly balanced withAdd()andDone() - Shared map access in
streamSubscribeis protected by initialization checks - The
closeChchannel is used correctly for shutdown signaling - The
atomic.Boolis used for the closed flag - Channel-based communication (
errCh,subDone) ensures thread-safe coordination
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Run the Go race detector to identify potential race conditions.
# Test: Build and run tests with race detection enabled.
go test -race ./pkg/grpc/default_api/ -run ^TestSubscribeTopicEvents$
Length of output: 760
Script:
#!/bin/bash
# Check the implementation of streamSubscribe and look for shared resource access
ast-grep --pattern 'func (a *api) streamSubscribe($_, $_) error {
$$$
}'
# Look for fields in the api struct that might be shared
ast-grep --pattern 'type api struct {
$$$
}'
# Search for any other methods that might access the same resources
rg "wg\." -A 3 pkg/grpc/default_api/
Length of output: 9847
136-142: Ensure metadata is properly initialized and used
In the callback for Subscribe, you ensure msg.Metadata is initialized. Verify that the metadata is correctly propagated and used throughout the message processing pipeline.
Run the following script to search for all usages of Metadata_key_pubsubName:
✅ Verification successful
Let me gather more context about how the metadata is used in the message processing pipeline.
Let me check the actual message processing pipeline and metadata usage.
Metadata initialization and propagation is properly implemented
The metadata handling is correctly implemented throughout the message processing pipeline:
- The metadata is properly initialized before setting the pubsubName
- The pubsubName metadata is consistently used in both the default and Dapr APIs
- The metadata is correctly propagated to cloud events in the message processing pipeline
- The publishMessageForStream function properly handles the message with its metadata
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of Metadata_key_pubsubName to ensure it's handled properly.
# Test: Find all references to Metadata_key_pubsubName.
rg 'Metadata_key_pubsubName'
Length of output: 760
Script:
#!/bin/bash
# Search for message processing related to metadata and pubsub
ast-grep --pattern 'func $_($_, msg *pubsub.NewMessage) $_ {
$$$
}'
# Also search for any tests related to metadata handling
rg -A 5 'TestSubscribe.*' --type go
Length of output: 1768
Script:
#!/bin/bash
# Check the publishMessageForStream implementation
rg -A 10 "publishMessageForStream" --type go
# Check how metadata is used in pubsub processing
rg -B 2 -A 5 "msg\.Metadata\[" --type go
Length of output: 5029
spec/proto/runtime/v1/runtime.proto (2)
73-76: Addition of 'SubscribeTopicEvents' RPC method is correct
The new RPC method SubscribeTopicEvents has been correctly added to the Runtime service, enabling Pub/Sub topic subscriptions via streaming.
792-792: Check compatibility of 'optional' label on 'dead_letter_topic'
The optional label is used on the scalar field dead_letter_topic. In proto3, the optional keyword for scalar fields requires protoc version 3.15 or later.
Confirm that your build environment and tooling support optional fields in proto3. If not, consider removing the optional label or updating your tooling to a compatible version.
| func main() { | ||
| flag.Parse() | ||
| if storeName == "" { | ||
| panic("storeName is empty.") | ||
| } | ||
| testDynamicSub() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in main function
Using panic for configuration validation is not a good practice. Consider returning an error and handling it gracefully.
-func main() {
+func run() error {
flag.Parse()
if storeName == "" {
- panic("storeName is empty.")
+ return fmt.Errorf("storeName is required")
}
- testDynamicSub()
+ return testDynamicSub()
+}
+
+func main() {
+ if err := run(); err != nil {
+ log.Fatalf("Error: %v", err)
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| func testDynamicSub() { | ||
| cli, err := client.NewClient() | ||
|
|
There was a problem hiding this comment.
Handle error from client creation
The error from NewClient() is not being checked.
func testDynamicSub() {
cli, err := client.NewClient()
+ if err != nil {
+ return fmt.Errorf("failed to create client: %w", err)
+ }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint
46-46: ineffectual assignment to err
(ineffassign)
| stop, err := cli.SubscribeWithHandler(context.Background(), client.SubscriptionRequest{ | ||
| PubsubName: storeName, | ||
| Topic: "hello", | ||
| Metadata: nil, | ||
| }, eventHandler) | ||
|
|
||
| //Use Subscribe API to subscribe to a topic. | ||
| sub, err := cli.Subscribe(context.Background(), client.SubscriptionRequest{ | ||
| PubsubName: storeName, | ||
| Topic: "topic1", | ||
| Metadata: nil, | ||
| }) | ||
|
|
||
| if err != nil { | ||
| log.Fatalf("failed to subscribe to topic: %v", err) | ||
| } |
There was a problem hiding this comment.
Improve error handling for subscriptions
The error from SubscribeWithHandler is not checked, and error handling is inconsistent between the two subscription methods.
- stop, err := cli.SubscribeWithHandler(context.Background(), client.SubscriptionRequest{
+ stop, err := cli.SubscribeWithHandler(context.Background(), client.SubscriptionRequest{
PubsubName: storeName,
Topic: "hello",
Metadata: nil,
}, eventHandler)
+ if err != nil {
+ return fmt.Errorf("failed to subscribe with handler: %w", err)
+ }
sub, err := cli.Subscribe(context.Background(), client.SubscriptionRequest{
PubsubName: storeName,
Topic: "topic1",
Metadata: nil,
})
if err != nil {
- log.Fatalf("failed to subscribe to topic: %v", err)
+ return fmt.Errorf("failed to subscribe to topic: %w", err)
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint
49-49: ineffectual assignment to err
(ineffassign)
sdk/go-sdk/client/subscribe.go
Outdated
| const ( | ||
| // SubscriptionResponseStatusSuccess means message is processed successfully. | ||
| SubscriptionResponseStatusSuccess SubscriptionResponseStatus = "SUCCESS" | ||
| // SubscriptionResponseStatusRetry means message to be retried by Dapr. |
There was a problem hiding this comment.
Correct the reference to "Dapr" in the comment
The comment on line 16 mentions "Dapr" instead of "Layotto", which may cause confusion.
Apply this diff to correct the comment:
- // SubscriptionResponseStatusRetry means message to be retried by Dapr.
+ // SubscriptionResponseStatusRetry means the message will be retried by Layotto.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SubscriptionResponseStatusRetry means message to be retried by Dapr. | |
| // SubscriptionResponseStatusRetry means the message will be retried by Layotto. |
sdk/go-sdk/client/subscribe.go
Outdated
| go func() { | ||
| defer s.Close() | ||
|
|
||
| for { | ||
| msg, err := s.Receive() | ||
| if err != nil { | ||
| if !s.closed.Load() { | ||
| logger.Printf("Error receiving messages from subscription pubsub=%s topic=%s, closing subscription: %s", | ||
| request.PubsubName, request.Topic, err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| go func() { | ||
| if err := msg.respondStatus(handler(msg.TopicEventRequest)); err != nil { | ||
| logger.Printf("Error responding to topic with event status pubsub=%s topic=%s message_id=%s: %s", | ||
| request.PubsubName, request.Topic, msg.Id, err) | ||
| } | ||
| }() | ||
| } | ||
| }() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid unbounded goroutines in SubscribeWithHandler
Starting a new goroutine for each message (line 81) may lead to unbounded goroutine growth if messages are received rapidly, potentially causing resource exhaustion.
Consider processing messages sequentially or using a worker pool to manage concurrency. Here's how you can process messages sequentially:
func (c *GRPCClient) SubscribeWithHandler(ctx context.Context, request SubscriptionRequest, handler SubscriptionHandleFunction) (func() error, error) {
s, err := c.Subscribe(ctx, request)
if err != nil {
return nil, err
}
go func() {
defer s.Close()
for {
msg, err := s.Receive()
if err != nil {
if !s.closed.Load() {
logger.Printf("Error receiving messages from subscription pubsub=%s topic=%s, closing subscription: %s",
request.PubsubName, request.Topic, err)
}
return
}
- go func() {
if err := msg.respondStatus(handler(msg.TopicEventRequest)); err != nil {
logger.Printf("Error responding to topic with event status pubsub=%s topic=%s message_id=%s: %s",
request.PubsubName, request.Topic, msg.Id, err)
}
- }()
}
}()
return s.Close, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go func() { | |
| defer s.Close() | |
| for { | |
| msg, err := s.Receive() | |
| if err != nil { | |
| if !s.closed.Load() { | |
| logger.Printf("Error receiving messages from subscription pubsub=%s topic=%s, closing subscription: %s", | |
| request.PubsubName, request.Topic, err) | |
| } | |
| return | |
| } | |
| go func() { | |
| if err := msg.respondStatus(handler(msg.TopicEventRequest)); err != nil { | |
| logger.Printf("Error responding to topic with event status pubsub=%s topic=%s message_id=%s: %s", | |
| request.PubsubName, request.Topic, msg.Id, err) | |
| } | |
| }() | |
| } | |
| }() | |
| go func() { | |
| defer s.Close() | |
| for { | |
| msg, err := s.Receive() | |
| if err != nil { | |
| if !s.closed.Load() { | |
| logger.Printf("Error receiving messages from subscription pubsub=%s topic=%s, closing subscription: %s", | |
| request.PubsubName, request.Topic, err) | |
| } | |
| return | |
| } | |
| if err := msg.respondStatus(handler(msg.TopicEventRequest)); err != nil { | |
| logger.Printf("Error responding to topic with event status pubsub=%s topic=%s message_id=%s: %s", | |
| request.PubsubName, request.Topic, msg.Id, err) | |
| } | |
| } | |
| }() |
| if len(initialRequest.GetPubsubName()) == 0 { | ||
| return errors.New("pubsubName is required") | ||
| } | ||
|
|
||
| if len(initialRequest.GetTopic()) == 0 { | ||
| return errors.New("topic is required") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate input parameters thoroughly
The checks for empty pubsubName and topic fields are important. However, consider trimming whitespace to prevent subscriptions with empty or whitespace-only names.
Apply this diff to trim whitespace:
if len(strings.TrimSpace(initialRequest.GetPubsubName())) == 0 {
return errors.New("pubsubName is required")
}
if len(strings.TrimSpace(initialRequest.GetTopic())) == 0 {
return errors.New("topic is required")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(initialRequest.GetPubsubName()) == 0 { | |
| return errors.New("pubsubName is required") | |
| } | |
| if len(initialRequest.GetTopic()) == 0 { | |
| return errors.New("topic is required") | |
| } | |
| if len(strings.TrimSpace(initialRequest.GetPubsubName())) == 0 { | |
| return errors.New("pubsubName is required") | |
| } | |
| if len(strings.TrimSpace(initialRequest.GetTopic())) == 0 { | |
| return errors.New("topic is required") | |
| } | |
| ``` | |
| Note: The suggestion assumes that the `strings` package is already imported in the file. If it's not, an additional import statement would need to be added at the top of the file: | |
| ```go | |
| import "strings" |
| conn.streamLock.Lock() | ||
| err = conn.stream.Send(&runtimev1pb.SubscribeTopicEventsResponse{ | ||
| SubscribeTopicEventsResponseType: &runtimev1pb.SubscribeTopicEventsResponse_EventMessage{ | ||
| EventMessage: envelope, | ||
| }, | ||
| }) | ||
| conn.streamLock.Unlock() |
There was a problem hiding this comment.
Handle errors when sending messages to the client
After sending the event message to the client with conn.stream.Send(...), the returned error err is not checked. If an error occurs during the send, it should be handled to prevent silent failures or unintended behavior.
Apply this diff to handle the error:
conn.streamLock.Lock()
err = conn.stream.Send(&runtimev1pb.SubscribeTopicEventsResponse{
SubscribeTopicEventsResponseType: &runtimev1pb.SubscribeTopicEventsResponse_EventMessage{
EventMessage: envelope,
},
})
conn.streamLock.Unlock()
+if err != nil {
+ return err
+}
var resp *runtimev1pb.SubscribeTopicEventsRequestProcessedCommittable suggestion skipped: line range outside the PR's diff.
spec/proto/runtime/v1/runtime.proto
Outdated
| @@ -4,6 +4,7 @@ package spec.proto.runtime.v1; | |||
|
|
|||
| import "google/protobuf/empty.proto"; | |||
| import "google/protobuf/any.proto"; | |||
| import "appcallback.proto"; | |||
There was a problem hiding this comment.
Fix missing import: 'appcallback.proto' does not exist
The import statement on line 7 references "appcallback.proto", but this file does not exist in the codebase. This will lead to compilation errors.
Ensure that appcallback.proto is added to the codebase or correct the import path if it's incorrectly specified.
🧰 Tools
🪛 buf
7-7: import "appcallback.proto": file does not exist
(COMPILE)
| // id is the unique identifier for the subscription request. | ||
| string id = 1; | ||
|
|
||
| // status is the result of the subscription request. | ||
| TopicEventResponse status = 2; | ||
| } |
There was a problem hiding this comment.
Undefined type: 'TopicEventResponse' in 'SubscribeTopicEventsRequestProcessed'
The field status in SubscribeTopicEventsRequestProcessed is of type TopicEventResponse, which is not defined in this file.
Since appcallback.proto is not properly imported, TopicEventResponse is undefined. Ensure that appcallback.proto is correctly imported and that TopicEventResponse is accessible.
| message SubscribeTopicEventsResponse { | ||
| oneof subscribe_topic_events_response_type { | ||
| SubscribeTopicEventsResponseInitial initial_response = 1; | ||
| TopicEventRequest event_message = 2; |
There was a problem hiding this comment.
Undefined type: 'TopicEventRequest' in 'SubscribeTopicEventsResponse'
The field event_message in SubscribeTopicEventsResponse is of type TopicEventRequest, which is not defined in this file.
Ensure that appcallback.proto is properly imported and that TopicEventRequest is defined and available for use.
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (15)
demo/pubsub/README.md (2)
Line range hint
5-35: Documentation needs enhancement for clarity and completeness.While the instructions are generally clear, consider the following improvements:
- Add an explanation of the differences between
appcallbackandSubscribeTopicEventsapproaches- Include prerequisites (Go version, required dependencies)
- Add troubleshooting section for common issues
- Document the significance of the
pub_subs_demostore nameHere's a suggested improvement:
+#################### Overview #################### +This demo showcases two different approaches for implementing pub/sub: +- appcallback: Uses application callbacks for handling messages +- SubscribeTopicEvents: Uses dynamic topic subscription + +Prerequisites: +- Go 1.16 or higher +- Layotto runtime dependencies + #################### Run pubsub demo with appcallback #################### +# This approach uses callback mechanisms for message handling cd ${project_path}/demo/pubsub/appcallback/ # 1. start subscriber go build -o subscriber /.subscriber -s pub_subs_demo # 2. start layotto cd ${project_path}/cmd/layotto go build -o layotto ./layotto start -c ../../configs/config_standalone.json # 3. start publisher cd ${project_path}/demo/pubsub/client/ go build -o publisher ./publisher -s pub_subs_demo #################### Run pubsub demo with SubscribeTopicEvents #################### +# This approach uses dynamic topic subscription # 1. start layotto cd ${project_path}/cmd/layotto go build -o layotto ./layotto start -c ../../configs/config_standalone.json cd ${project_path}/demo/pubsub/dynamic/ # 2. start subscriber go build -o subscriber /.subscriber -s pub_subs_demo # 3. start publisher cd ${project_path}/demo/pubsub/client/ go build -o publisher ./publisher -s pub_subs_demo +#################### Troubleshooting #################### +Common issues: +1. Ensure all components use the same store name (-s parameter) +2. Verify Layotto is running before starting subscriber/publisher +3. Check logs in case of connection issues
Line range hint
5-35: Fix formatting inconsistencies in shell commands.There are some inconsistent indentations and extra spaces in the shell commands.
Apply this formatting fix:
#################### Run pubsub demo with appcallback #################### cd ${project_path}/demo/pubsub/appcallback/ # 1. start subscriber go build -o subscriber -/.subscriber -s pub_subs_demo +./subscriber -s pub_subs_demo # 2. start layotto cd ${project_path}/cmd/layotto go build -o layotto ./layotto start -c ../../configs/config_standalone.json # 3. start publisher -cd ${project_path}/demo/pubsub/client/ +cd ${project_path}/demo/pubsub/client/ go build -o publisher -./publisher -s pub_subs_demo +./publisher -s pub_subs_demo #################### Run pubsub demo with SubscribeTopicEvents #################### # 1. start layotto cd ${project_path}/cmd/layotto go build -o layotto ./layotto start -c ../../configs/config_standalone.json cd ${project_path}/demo/pubsub/dynamic/ # 2. start subscriber go build -o subscriber -/.subscriber -s pub_subs_demo +./subscriber -s pub_subs_demo # 3. start publisher -cd ${project_path}/demo/pubsub/client/ +cd ${project_path}/demo/pubsub/client/ go build -o publisher -./publisher -s pub_subs_demo +./publisher -s pub_subs_demodemo/pubsub/server/dynamic/subscribe_server.go (2)
92-96: Enhance event handler with validation and structured loggingThe current implementation could benefit from better validation and logging.
Consider these improvements:
func eventHandler(request *runtimev1pb.TopicEventRequest) client.SubscriptionResponseStatus { + if request == nil { + log.Printf("Error: received nil request") + return client.SubscriptionResponseStatusRetry + } + + // Validate required fields + if request.PubsubName == "" || request.Topic == "" { + log.Printf("Error: missing required fields in request") + return client.SubscriptionResponseStatusDrop + } + - log.Printf(">>[SubscribeWithHandler API] Received message\n") - log.Printf("event - PubsubName: %s, Topic: %s, ID: %s, Data: %s\n", request.PubsubName, request.Topic, request.Id, request.Data) + // Use structured logging + log.Printf("[SubscribeWithHandler] Received message: {PubsubName: %q, Topic: %q, ID: %q, DataLen: %d}", + request.PubsubName, request.Topic, request.Id, len(request.Data)) + return client.SubscriptionResponseStatusSuccess }
1-96: Consider enhancing the demo with additional examples and documentationAs this is a demo implementation, it would be valuable to:
- Add comments explaining the differences between
SubscribeandSubscribeWithHandler- Include examples of handling different message types
- Add configuration management for timeouts, retries, etc.
Would you like me to provide examples for these enhancements?
🧰 Tools
🪛 golangci-lint
46-46: ineffectual assignment to err
(ineffassign)
49-49: ineffectual assignment to err
(ineffassign)
pkg/grpc/default_api/api_subscribe_test.go (3)
35-68: Enhance test coverage with additional scenariosThe test currently only covers the duplicate initial request case. Consider adding test cases for:
- Successful subscription
- Invalid pubsub name
- Invalid topic name
- Subscription with different metadata
- Error scenarios from the pubsub component
Also, verify the specific error type/message returned and ensure proper cleanup of resources.
Would you like me to help generate additional test cases?
110-113: Extract subscriber key format into a constantThe hardcoded string
"___test||layotto"appears to be using a specific format for the subscriber key. Consider extracting this into a constant or helper function to make the format more maintainable.+ const subscriberKeyFormat = "____%s||%s" + func formatSubscriberKey(pubsubName, topic string) string { + return fmt.Sprintf(subscriberKeyFormat, pubsubName, topic) + }
70-134: Add test cases for error scenariosThe test only covers the success case. Consider adding test cases for:
- Failed message publishing
- Timeout scenarios
- Invalid message format
- Missing/invalid subscriber
- Error responses from subscribers
Would you like me to help generate additional test cases?
🧰 Tools
🪛 golangci-lint
122-122: S1005: unnecessary assignment to the blank identifier
(gosimple)
sdk/go-sdk/client/client.go (2)
61-64: Enhance documentation with examples and error scenarios.The
Subscribemethod's documentation is clear but could be more comprehensive. Consider adding:
- Example usage
- Common error scenarios
- Details about the
SubscriptionRequeststructure- Clarification on when/how to close the
SubscriptionAdd expanded documentation:
// Subscribe subscribes to a pubsub topic and streams messages to the returned Subscription. // Subscription must be closed after finishing with subscribing. +// +// Example usage: +// sub, err := client.Subscribe(ctx, SubscriptionRequest{ +// PubsubName: "pubsub", +// Topic: "orders", +// }) +// if err != nil { +// return err +// } +// defer sub.Close() +// +// Common errors: +// - Context cancellation +// - Invalid pubsub name +// - Topic does not exist
65-68: Consider adding context about handler error handling.The
SubscribeWithHandlermethod's documentation should clarify:
- How handler errors are processed
- Whether the handler is called sequentially or concurrently
- The behavior when the handler panics
Add expanded documentation:
// SubscribeWithHandler subscribes to a pubsub topic and calls the given handler on topic events. // The returned cancel function must be called after finishing with subscribing. +// +// The handler function is called sequentially for each message. If the handler returns an error, +// the subscription continues but the error is logged. Panics in the handler are recovered. +// +// Example usage: +// cancel, err := client.SubscribeWithHandler(ctx, request, func(msg *TopicEvent) error { +// // Process message +// return nil +// }) +// if err != nil { +// return err +// } +// defer cancel()sdk/go-sdk/client/subscribe.go (1)
93-99: Make 'Close' idempotent by not returning an error if already closedReturning an error when
Closeis called multiple times may not be necessary. MakingCloseidempotent improves usability and aligns with common practice.Update the
Closemethod to returnnilif the subscription is already closed:func (s *Subscription) Close() error { - if !s.closed.CompareAndSwap(false, true) { - return errors.New("subscription already closed") + if !atomic.CompareAndSwapInt32(&s.closed, 0, 1) { + return nil } return s.stream.CloseSend() }pkg/grpc/default_api/api_subscribe.go (4)
219-225: Avoid reusing theerrvariableReassigning
errwhen sending the stream response can overwrite the previous error, leading to confusion. It's better to use a separate variable for the error fromconn.stream.Send.Apply this diff to use a different variable:
err = conn.stream.Send(&runtimev1pb.SubscribeTopicEventsResponse{ SubscribeTopicEventsResponseType: &runtimev1pb.SubscribeTopicEventsResponse_EventMessage{ EventMessage: envelope, }, }) +if err != nil { + return err +}Alternatively:
-err = conn.stream.Send(&runtimev1pb.SubscribeTopicEventsResponse{ +sendErr := conn.stream.Send(&runtimev1pb.SubscribeTopicEventsResponse{ SubscribeTopicEventsResponseType: &runtimev1pb.SubscribeTopicEventsResponse_EventMessage{ EventMessage: envelope, }, }) +if sendErr != nil { + return sendErr +}
178-179: Rename variable to avoid shadowingThe variable
sins, ok := status.FromError(err)shadows the imported packagestatus. This can lead to confusion and potential errors.Rename the variable to avoid shadowing:
-s, ok := status.FromError(err) +st, ok := status.FromError(err)
112-118: Reduce unnecessary logging in subscriptionThe loop logging all subscriptions each time a new subscription is added can be inefficient, especially with many subscriptions.
Modify the logging to only include the new subscription:
if len(a.topicPerComponent) > 0 { - for pubsubName, v := range a.topicPerComponent { - topics := []string{} - for topic := range v.topic2Details { - topics = append(topics, topic) - } - log.DefaultLogger.Infof("[runtime][streamSubscribe]app is subscribed to the following topics: %v through pubsub=%s", topics, pubsubName) - } + log.DefaultLogger.Infof("[runtime][streamSubscribe]app subscribed to topic '%s' through pubsub='%s'", initialRequest.Topic, initialRequest.PubsubName) }
121-123: Check for uninitialized pubsub componentsThe error message in line 122 might not provide enough detail. If
a.pubSubs[initialRequest.PubsubName]isnil, it could be due to misconfiguration or initialization issues.Consider providing a more informative error message or handling the initialization earlier.
if a.pubSubs[initialRequest.PubsubName] == nil { - return errors.New("pubsub " + initialRequest.PubsubName + " is not initialized.") + return fmt.Errorf("pubsub component '%s' is not initialized or configured", initialRequest.PubsubName) }spec/proto/runtime/v1/runtime.proto (1)
814-816: Inconsistent Message Name in CommentThe comment refers to
SubscribeTopicEventsResponseInitialAlpha1, but the message is defined asSubscribeTopicEventsResponseInitial. Please update the comment or the message name to ensure consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (5)
pkg/mock/runtime/runtime.pb.gois excluded by!**/*.pb.gospec/proto/extension/v1/s3/oss.pb.gois excluded by!**/*.pb.gospec/proto/extension/v1/s3/oss_grpc.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
components/pluggable/grpc_mock.go(0 hunks)components/sequencer/snowflake/snowflake.go(1 hunks)components/sequencer/snowflake/snowflake_sequencer.go(0 hunks)components/sequencer/snowflake/snowflake_sequencer_test.go(0 hunks)components/sequencer/snowflake/snowflake_test.go(0 hunks)demo/pubsub/README.md(2 hunks)demo/pubsub/server/dynamic/subscribe_server.go(1 hunks)pkg/grpc/default_api/api.go(3 hunks)pkg/grpc/default_api/api_pubsub.go(2 hunks)pkg/grpc/default_api/api_subscribe.go(1 hunks)pkg/grpc/default_api/api_subscribe_test.go(1 hunks)pkg/mock/components/lock/lock.go(1 hunks)sdk/go-sdk/client/client.go(1 hunks)sdk/go-sdk/client/subscribe.go(1 hunks)spec/proto/runtime/v1/runtime.proto(3 hunks)
💤 Files with no reviewable changes (4)
- components/pluggable/grpc_mock.go
- components/sequencer/snowflake/snowflake_sequencer.go
- components/sequencer/snowflake/snowflake_sequencer_test.go
- components/sequencer/snowflake/snowflake_test.go
✅ Files skipped from review due to trivial changes (2)
- components/sequencer/snowflake/snowflake.go
- pkg/mock/components/lock/lock.go
🧰 Additional context used
🪛 golangci-lint
demo/pubsub/server/dynamic/subscribe_server.go
46-46: ineffectual assignment to err
(ineffassign)
49-49: ineffectual assignment to err
(ineffassign)
pkg/grpc/default_api/api.go
93-93: field closed is unused
(unused)
pkg/grpc/default_api/api_pubsub.go
156-156: ineffectual assignment to err
(ineffassign)
pkg/grpc/default_api/api_subscribe.go
213-213: ineffectual assignment to err
(ineffassign)
pkg/grpc/default_api/api_subscribe_test.go
122-122: S1005: unnecessary assignment to the blank identifier
(gosimple)
🪛 buf
spec/proto/runtime/v1/runtime.proto
7-7: import "appcallback.proto": file does not exist
(COMPILE)
🔇 Additional comments (4)
demo/pubsub/server/dynamic/subscribe_server.go (1)
1-29: LGTM!
The package structure and imports are well-organized and appropriate for the implementation.
pkg/grpc/default_api/api.go (1)
90-95: Verify proper cleanup implementation for concurrent operations.
The new fields (streamer, closed, closeCh, wg) suggest concurrent operation management for subscriptions. Ensure proper cleanup is implemented in a Close or Shutdown method.
🧰 Tools
🪛 golangci-lint
93-93: field closed is unused
(unused)
sdk/go-sdk/client/client.go (1)
61-68: Verify the cleanup pattern consistency across the codebase.
The two new subscription methods use different cleanup patterns:
Subscribereturns aSubscriptionobject that must be closedSubscribeWithHandlerreturns a cancel function that must be called
While both patterns are valid, we should ensure consistency with similar patterns in the codebase.
✅ Verification successful
The cleanup patterns are consistent with their respective use cases
The implementation shows that both cleanup patterns are well-designed for their specific use cases:
-
Subscribereturns a*SubscriptionwithClose()because it gives full control to the user for manual message handling and cleanup. TheClose()method properly handles concurrent access and prevents double-closing. -
SubscribeWithHandlerreturns a cancel function because it manages the subscription lifecycle internally in a goroutine. The cancel function is actually the sameClose()method, maintaining consistency under the hood.
This is a good example of providing two different APIs for different use cases while maintaining internal consistency in the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar cleanup patterns in the codebase
echo "Searching for Close() patterns..."
rg -A 2 "func.*\) \(\*.*\)" | rg "Close\(\)"
echo "Searching for cancel function patterns..."
rg "return func\(\) error"
Length of output: 245
Script:
#!/bin/bash
# Let's try a more comprehensive search for cleanup patterns
# Search for Close method definitions
echo "=== Searching for Close method definitions ==="
ast-grep --pattern 'func ($x *$_) Close() {
$$$
}'
# Search for function returns with cleanup
echo -e "\n=== Searching for cleanup function returns ==="
ast-grep --pattern 'return func() {
$$$
}'
# Search for subscription-related types and methods
echo -e "\n=== Searching for subscription types and methods ==="
rg -l "type.*Subscription"
# Look for other cleanup patterns in Go files
echo -e "\n=== Searching for other cleanup patterns ==="
rg -t go "must be (closed|called)" -A 2
Length of output: 3190
Script:
#!/bin/bash
# Let's examine the subscription-related files more closely
echo "=== Examining subscription.go implementation ==="
cat sdk/go-sdk/client/subscribe.go
echo -e "\n=== Looking for other Close() methods in subscription files ==="
rg -t go "func.*Close\(\)" sdk/go-sdk/client/subscribe.go
echo -e "\n=== Looking for similar pubsub patterns in the codebase ==="
rg -t go "type.*PubSub" -A 5
Length of output: 7097
pkg/grpc/default_api/api_subscribe.go (1)
54-61: Verify proper cleanup when a.closeCh is closed
Ensure that all resources and goroutines are properly cleaned up when a.closeCh is closed to prevent leaks.
Run the following script to check for potential goroutine leaks related to a.closeCh:
This will display the context around a.closeCh usage for manual verification.
✅ Verification successful
Let me gather more context about the cleanup mechanisms.
Let me check one more aspect about the API struct and its initialization.
Based on the gathered evidence, I can now provide the final response:
Proper cleanup is implemented for goroutines and channels
The code shows appropriate cleanup mechanisms:
- Uses
WaitGroupto track and wait for goroutines (a.wg.Add(2)with correspondingDonecalls) subDonechannel is properly closed withdefer close(subDone)- Both goroutines have proper termination conditions through select statements
- Each goroutine has
defer a.wg.Done()to ensure WaitGroup is decremented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all goroutines related to 'a.closeCh' are properly terminated.
# Search for uses of 'a.closeCh' to ensure proper handling.
rg 'a\.closeCh' -A 5 -B 5
Length of output: 595
Script:
#!/bin/bash
# Check for WaitGroup usage and close channel initialization
rg "closeCh.*=.*make\(chan" -A 5 -B 5
# Check the struct definition and initialization
ast-grep --pattern 'type API struct {
$$$
closeCh $_
$$$
}'
# Look for cleanup/close functions
rg "func.*Close|Done" -A 10 pkg/grpc/default_api/
Length of output: 5983
Script:
#!/bin/bash
# Check API struct initialization and Close method
ast-grep --pattern 'func New$_($_) *API {
$$$
}'
# Look for Close method implementation
ast-grep --pattern 'func ($_ *API) Close() {
$$$
}'
# Check for any other cleanup related code
rg "defer.*close\(" -A 2 -B 2 pkg/grpc/default_api/
Length of output: 543
| func init() { | ||
| flag.StringVar(&storeName, "s", "", "set `storeName`") | ||
| } | ||
|
|
||
| func main() { | ||
| flag.Parse() | ||
| if storeName == "" { | ||
| panic("storeName is empty.") | ||
| } | ||
| testDynamicSub() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and adding graceful shutdown
The current implementation has several areas for improvement:
- The panic message could be more descriptive
- Consider implementing graceful shutdown handling
- The
testDynamicSub()errors should be handled
Consider applying these improvements:
func main() {
flag.Parse()
if storeName == "" {
- panic("storeName is empty.")
+ log.Fatal("storeName is required: please provide it using the -s flag")
}
- testDynamicSub()
+
+ // Setup graceful shutdown
+ ctx, cancel := context.WithCancel(context.Background())
+ defer cancel()
+
+ // Handle shutdown signals
+ go func() {
+ sigCh := make(chan os.Signal, 1)
+ signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM)
+ <-sigCh
+ cancel()
+ }()
+
+ if err := testDynamicSub(); err != nil {
+ log.Fatal("Failed to run subscription test:", err)
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| func testDynamicSub() { | ||
| cli, err := client.NewClient() | ||
|
|
||
| // Use SubscribeWithHandler API to subscribe to a topic. | ||
| stop, err := cli.SubscribeWithHandler(context.Background(), client.SubscriptionRequest{ | ||
| PubsubName: storeName, | ||
| Topic: "hello", | ||
| Metadata: nil, | ||
| }, eventHandler) | ||
|
|
||
| //Use Subscribe API to subscribe to a topic. | ||
| sub, err := cli.Subscribe(context.Background(), client.SubscriptionRequest{ | ||
| PubsubName: storeName, | ||
| Topic: "topic1", | ||
| Metadata: nil, | ||
| }) | ||
|
|
||
| if err != nil { | ||
| log.Fatalf("failed to subscribe to topic: %v", err) | ||
| } | ||
|
|
||
| msg, err := sub.Receive() | ||
| if err != nil { | ||
| log.Fatalf("Error receiving message: %v", err) | ||
| } | ||
| log.Printf(">>[Subscribe API]Received message\n") | ||
| log.Printf("event - PubsubName: %s, Topic: %s, ID: %s, Data: %s\n", msg.PubsubName, msg.Topic, msg.Id, msg.Data) | ||
|
|
||
| // Use _MUST_ always signal the result of processing the message, else the | ||
| // message will not be considered as processed and will be redelivered or | ||
| // dead lettered. | ||
| if err := msg.Success(); err != nil { | ||
| log.Fatalf("error sending message success: %v", err) | ||
| } | ||
|
|
||
| time.Sleep(time.Second * 10) | ||
|
|
||
| if err := errors.Join(stop(), sub.Close()); err != nil { | ||
| log.Fatal(err) | ||
| } | ||
|
|
||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| cli.Close() | ||
| } |
There was a problem hiding this comment.
Fix error handling and improve resource management
Several critical issues need to be addressed:
- Missing error checks for client creation and subscription
- Hardcoded sleep duration
- Redundant error check at the end
- Resource cleanup could be improved
Apply these fixes:
-func testDynamicSub() {
+func testDynamicSub() error {
cli, err := client.NewClient()
+ if err != nil {
+ return fmt.Errorf("failed to create client: %w", err)
+ }
+ defer cli.Close()
// Use SubscribeWithHandler API to subscribe to a topic.
stop, err := cli.SubscribeWithHandler(context.Background(), client.SubscriptionRequest{
PubsubName: storeName,
Topic: "hello",
Metadata: nil,
}, eventHandler)
+ if err != nil {
+ return fmt.Errorf("failed to subscribe with handler: %w", err)
+ }
+ defer stop()
//Use Subscribe API to subscribe to a topic.
sub, err := cli.Subscribe(context.Background(), client.SubscriptionRequest{
@@ -58,6 +58,7 @@
Topic: "topic1",
Metadata: nil,
})
+ defer sub.Close()
if err != nil {
log.Fatalf("failed to subscribe to topic: %v", err)
@@ -80,13 +81,7 @@
time.Sleep(time.Second * 10)
- if err := errors.Join(stop(), sub.Close()); err != nil {
- log.Fatal(err)
- }
-
- if err != nil {
- panic(err)
- }
- cli.Close()
+ return nil
}Additionally, consider replacing the hardcoded sleep with a configurable duration or context-based cancellation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func testDynamicSub() { | |
| cli, err := client.NewClient() | |
| // Use SubscribeWithHandler API to subscribe to a topic. | |
| stop, err := cli.SubscribeWithHandler(context.Background(), client.SubscriptionRequest{ | |
| PubsubName: storeName, | |
| Topic: "hello", | |
| Metadata: nil, | |
| }, eventHandler) | |
| //Use Subscribe API to subscribe to a topic. | |
| sub, err := cli.Subscribe(context.Background(), client.SubscriptionRequest{ | |
| PubsubName: storeName, | |
| Topic: "topic1", | |
| Metadata: nil, | |
| }) | |
| if err != nil { | |
| log.Fatalf("failed to subscribe to topic: %v", err) | |
| } | |
| msg, err := sub.Receive() | |
| if err != nil { | |
| log.Fatalf("Error receiving message: %v", err) | |
| } | |
| log.Printf(">>[Subscribe API]Received message\n") | |
| log.Printf("event - PubsubName: %s, Topic: %s, ID: %s, Data: %s\n", msg.PubsubName, msg.Topic, msg.Id, msg.Data) | |
| // Use _MUST_ always signal the result of processing the message, else the | |
| // message will not be considered as processed and will be redelivered or | |
| // dead lettered. | |
| if err := msg.Success(); err != nil { | |
| log.Fatalf("error sending message success: %v", err) | |
| } | |
| time.Sleep(time.Second * 10) | |
| if err := errors.Join(stop(), sub.Close()); err != nil { | |
| log.Fatal(err) | |
| } | |
| if err != nil { | |
| panic(err) | |
| } | |
| cli.Close() | |
| } | |
| func testDynamicSub() error { | |
| cli, err := client.NewClient() | |
| if err != nil { | |
| return fmt.Errorf("failed to create client: %w", err) | |
| } | |
| defer cli.Close() | |
| // Use SubscribeWithHandler API to subscribe to a topic. | |
| stop, err := cli.SubscribeWithHandler(context.Background(), client.SubscriptionRequest{ | |
| PubsubName: storeName, | |
| Topic: "hello", | |
| Metadata: nil, | |
| }, eventHandler) | |
| if err != nil { | |
| return fmt.Errorf("failed to subscribe with handler: %w", err) | |
| } | |
| defer stop() | |
| //Use Subscribe API to subscribe to a topic. | |
| sub, err := cli.Subscribe(context.Background(), client.SubscriptionRequest{ | |
| PubsubName: storeName, | |
| Topic: "topic1", | |
| Metadata: nil, | |
| }) | |
| defer sub.Close() | |
| if err != nil { | |
| log.Fatalf("failed to subscribe to topic: %v", err) | |
| } | |
| msg, err := sub.Receive() | |
| if err != nil { | |
| log.Fatalf("Error receiving message: %v", err) | |
| } | |
| log.Printf(">>[Subscribe API]Received message\n") | |
| log.Printf("event - PubsubName: %s, Topic: %s, ID: %s, Data: %s\n", msg.PubsubName, msg.Topic, msg.Id, msg.Data) | |
| // Use _MUST_ always signal the result of processing the message, else the | |
| // message will not be considered as processed and will be redelivered or | |
| // dead lettered. | |
| if err := msg.Success(); err != nil { | |
| log.Fatalf("error sending message success: %v", err) | |
| } | |
| time.Sleep(time.Second * 10) | |
| return nil | |
| } |
🧰 Tools
🪛 golangci-lint
46-46: ineffectual assignment to err
(ineffassign)
49-49: ineffectual assignment to err
(ineffassign)
| go func() { | ||
| time.Sleep(1 * time.Second) | ||
| ch, _ := apiForTest.streamer.subscribers["___test||layotto"].publishResponses["1"] | ||
| ch <- &runtimev1pb.SubscribeTopicEventsRequestProcessed{ | ||
| Id: "1", | ||
| Status: &runtimev1pb.TopicEventResponse{ | ||
| Status: runtimev1pb.TopicEventResponse_SUCCESS, | ||
| }, | ||
| } | ||
| }() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace time.Sleep with proper synchronization
Using time.Sleep in tests can lead to flaky behavior. Consider using channels or sync.WaitGroup for proper synchronization.
Here's a suggested improvement:
+ done := make(chan struct{})
go func() {
- time.Sleep(1 * time.Second)
ch, _ := apiForTest.streamer.subscribers["___test||layotto"].publishResponses["1"]
ch <- &runtimev1pb.SubscribeTopicEventsRequestProcessed{
Id: "1",
Status: &runtimev1pb.TopicEventResponse{
Status: runtimev1pb.TopicEventResponse_SUCCESS,
},
}
+ close(done)
}()
err = apiForTest.publishMessageForStream(context.Background(), msg, pubsubName)
assert.Nil(t, err)
+ <-done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go func() { | |
| time.Sleep(1 * time.Second) | |
| ch, _ := apiForTest.streamer.subscribers["___test||layotto"].publishResponses["1"] | |
| ch <- &runtimev1pb.SubscribeTopicEventsRequestProcessed{ | |
| Id: "1", | |
| Status: &runtimev1pb.TopicEventResponse{ | |
| Status: runtimev1pb.TopicEventResponse_SUCCESS, | |
| }, | |
| } | |
| }() | |
| done := make(chan struct{}) | |
| go func() { | |
| ch, _ := apiForTest.streamer.subscribers["___test||layotto"].publishResponses["1"] | |
| ch <- &runtimev1pb.SubscribeTopicEventsRequestProcessed{ | |
| Id: "1", | |
| Status: &runtimev1pb.TopicEventResponse{ | |
| Status: runtimev1pb.TopicEventResponse_SUCCESS, | |
| }, | |
| } | |
| close(done) | |
| }() | |
| err = apiForTest.publishMessageForStream(context.Background(), msg, pubsubName) | |
| assert.Nil(t, err) | |
| <-done |
🧰 Tools
🪛 golangci-lint
122-122: S1005: unnecessary assignment to the blank identifier
(gosimple)
| @@ -147,7 +152,9 @@ func NewAPI( | |||
| sendToOutputBindingFn: sendToOutputBindingFn, | |||
| secretStores: secretStores, | |||
| json: jsoniter.ConfigFastest, | |||
| //closeCh: make(chan struct{}), | |||
There was a problem hiding this comment.
Uncomment the closeCh initialization.
The closeCh field is declared but its initialization is commented out. This channel is likely needed for proper shutdown signaling in the subscription implementation.
-//closeCh: make(chan struct{}),
+closeCh: make(chan struct{}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //closeCh: make(chan struct{}), | |
| closeCh: make(chan struct{}), |
sdk/go-sdk/client/subscribe.go
Outdated
| logger.Printf("Error receiving messages from subscription pubsub=%s topic=%s, closing subscription: %s", | ||
| request.PubsubName, request.Topic, err) | ||
| } |
There was a problem hiding this comment.
Define or import 'logger' to enable logging
The code uses logger.Printf, but logger is not defined in this file. This will lead to compilation errors due to the undefined logger variable.
Consider importing the standard log package and replacing logger with log, or ensure that logger is properly defined.
Apply this diff:
package client
import (
"context"
"errors"
"fmt"
+ "log"
"sync"
"sync/atomic"
runtimev1pb "mosn.io/layotto/spec/proto/runtime/v1"
)
...
if !s.closed.Load() {
- logger.Printf("Error receiving messages from subscription pubsub=%s topic=%s, closing subscription: %s",
+ log.Printf("Error receiving messages from subscription pubsub=%s topic=%s, closing subscription: %s",
request.PubsubName, request.Topic, err)
}
...
if err := msg.respondStatus(handler(msg.TopicEventRequest)); err != nil {
- logger.Printf("Error responding to topic with event status pubsub=%s topic=%s message_id=%s: %s",
+ log.Printf("Error responding to topic with event status pubsub=%s topic=%s message_id=%s: %s",
request.PubsubName, request.Topic, msg.Id, err)
}Also applies to: 83-85
| // 5. Check result | ||
| return retryStrategy(err, resp.Status, cloudEvent) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure retryStrategy receives appropriate error values
Since err may not accurately reflect the previous operations due to reassignment, ensure that retryStrategy receives the correct error value.
Consider modifying the return statement:
-return retryStrategy(err, resp.Status, cloudEvent)
+return retryStrategy(nil, resp.Status, cloudEvent)Or, if handling errors as suggested earlier:
// After checking errors from previous operations
return retryStrategy(nil, resp.Status, cloudEvent)| return fmt.Errorf("no streamer subscribed to pubsub %q topic %q", pubsubName, msg.Topic) | ||
| } | ||
|
|
||
| envelope, cloudEvent, err := a.envelopeFromSubscriptionMessage(ctx, msg) |
There was a problem hiding this comment.
Handle error returned by envelopeFromSubscriptionMessage
The error returned by a.envelopeFromSubscriptionMessage(ctx, msg) is not checked, and err is overwritten at line 219. This could lead to unhandled errors and unexpected behavior.
Apply this diff to handle the error:
envelope, cloudEvent, err := a.envelopeFromSubscriptionMessage(ctx, msg)
+if err != nil {
+ return err
+}
err = conn.stream.Send(&runtimev1pb.SubscribeTopicEventsResponse{
SubscribeTopicEventsResponseType: &runtimev1pb.SubscribeTopicEventsResponse_EventMessage{
EventMessage: envelope,
},
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| envelope, cloudEvent, err := a.envelopeFromSubscriptionMessage(ctx, msg) | |
| envelope, cloudEvent, err := a.envelopeFromSubscriptionMessage(ctx, msg) | |
| if err != nil { | |
| return err | |
| } |
🧰 Tools
🪛 golangci-lint
213-213: ineffectual assignment to err
(ineffassign)
spec/proto/runtime/v1/runtime.proto
Outdated
| @@ -4,6 +4,7 @@ package spec.proto.runtime.v1; | |||
|
|
|||
| import "google/protobuf/empty.proto"; | |||
| import "google/protobuf/any.proto"; | |||
| import "appcallback.proto"; | |||
There was a problem hiding this comment.
Critical Issue: 'appcallback.proto' Import Not Found
The imported file appcallback.proto does not exist in the codebase, leading to compilation errors. Please ensure that appcallback.proto is included in the repository or adjust the import path accordingly.
🧰 Tools
🪛 buf
7-7: import "appcallback.proto": file does not exist
(COMPILE)
| string id = 1; | ||
|
|
||
| // status is the result of the subscription request. | ||
| TopicEventResponse status = 2; |
There was a problem hiding this comment.
Undefined Type TopicEventResponse Referenced
The type TopicEventResponse used in SubscribeTopicEventsRequestProcessed is not defined or imported in the current .proto file. This will cause compilation errors. Please define TopicEventResponse or import the appropriate .proto file where it is defined.
| message SubscribeTopicEventsResponse { | ||
| oneof subscribe_topic_events_response_type { | ||
| SubscribeTopicEventsResponseInitial initial_response = 1; | ||
| TopicEventRequest event_message = 2; |
There was a problem hiding this comment.
Undefined Type TopicEventRequest Referenced
The type TopicEventRequest used in SubscribeTopicEventsResponse is not defined or imported in the current .proto file. This will lead to compilation errors. Ensure that TopicEventRequest is defined and properly imported from the correct .proto file.
# Conflicts: # sdk/go-sdk/client/client.go
CrazyHZM
force-pushed
the
feat/support_SubscribeTopicEvents
branch
from
ec54b55
to
b9ab1e7
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (11)
demo/pubsub/README.md (2)
Line range hint
5-20: Improve formatting and clarity of the appcallback demo instructions.The instructions are functional but could benefit from improved formatting and clarity:
- Inconsistent indentation in shell commands
- Missing explanation of what
pub_subs_demorepresents- Unclear whether commands should be run sequentially or in separate terminals
Consider applying these improvements:
#################### Run pubsub demo with appcallback #################### cd ${project_path}/demo/pubsub/appcallback/ -# 1. start subscriber +# 1. Start subscriber (in terminal 1) go build -o subscriber -/.subscriber -s pub_subs_demo +./subscriber -s pub_subs_demo # pub_subs_demo is the store name -# 2. start layotto +# 2. Start layotto (in terminal 2) cd ${project_path}/cmd/layotto go build -o layotto ./layotto start -c ../../configs/config_standalone.json -# 3. start publisher - cd ${project_path}/demo/pubsub/client/ +# 3. Start publisher (in terminal 3) +cd ${project_path}/demo/pubsub/client/ go build -o publisher - ./publisher -s pub_subs_demo +./publisher -s pub_subs_demo # Use the same store name as subscriber
Line range hint
5-35: Consider adding prerequisite information and troubleshooting section.The instructions would benefit from additional context to help users succeed:
- Prerequisites (Go version, required dependencies)
- Expected output for each step
- Common issues and troubleshooting steps
- Explanation of the differences between
appcallbackandSubscribeTopicEventsapproachesWould you like me to help draft these additional sections?
demo/pubsub/server/dynamic/subscribe_server.go (3)
31-35: Enhance configuration documentationConsider adding documentation for:
- The
storeNamevariable explaining its purpose and expected values- The flag description to clarify what a valid store name looks like and its significance
-var storeName string +// storeName specifies the name of the pub/sub store to connect to +var storeName string func init() { - flag.StringVar(&storeName, "s", "", "set `storeName`") + flag.StringVar(&storeName, "s", "", "name of the pub/sub store (required)") }
80-80: Make sleep duration configurableThe hardcoded sleep duration should be configurable:
+const defaultWaitDuration = 10 * time.Second + +func init() { + // ... existing init code ... + flag.DurationVar(&waitDuration, "w", defaultWaitDuration, "duration to wait for messages") +} - time.Sleep(time.Second * 10) + time.Sleep(waitDuration)
92-96: Enhance event handler robustnessThe event handler should:
- Handle nil requests
- Validate required fields
- Use structured logging
func eventHandler(request *runtimev1pb.TopicEventRequest) client.SubscriptionResponseStatus { + if request == nil { + log.Printf("Error: received nil request") + return client.SubscriptionResponseStatusRetry + } + + if request.Topic == "" || request.PubsubName == "" { + log.Printf("Error: missing required fields in request") + return client.SubscriptionResponseStatusDrop + } + - log.Printf(">>[SubscribeWithHandler API] Received message\n") - log.Printf("event - PubsubName: %s, Topic: %s, ID: %s, Data: %s\n", request.PubsubName, request.Topic, request.Id, request.Data) + log.Printf("Received message: pubsub=%q topic=%q id=%q data=%q", + request.PubsubName, request.Topic, request.Id, request.Data) + return client.SubscriptionResponseStatusSuccess }pkg/grpc/default_api/api_subscribe_test.go (3)
36-36: Improve test name clarityThe test name "SubscribeTopicEvents duplicate initial request" could be more descriptive about the expected behavior.
- t.Run("SubscribeTopicEvents duplicate initial request", func(t *testing.T) { + t.Run("SubscribeTopicEvents should fail when receiving duplicate initial subscription request", func(t *testing.T) {
71-71: Improve test name specificityThe test name "publish success" could be more descriptive about what constitutes success.
- t.Run("publish success", func(t *testing.T) { + t.Run("should successfully publish message and receive acknowledgment", func(t *testing.T) {
85-92: Extract cloud event setup to a helper functionThe cloud event setup code could be reused across different test cases. Consider extracting it to a helper function.
func createTestCloudEvent(id string) map[string]interface{} { return map[string]interface{}{ pubsub.IDField: id, pubsub.SourceField: "source", pubsub.DataContentTypeField: "content-type", pubsub.TypeField: "type", pubsub.SpecVersionField: "v1.0.0", pubsub.DataBase64Field: "bGF5b3R0bw==", } } func createTestMessage(cloudEvent map[string]interface{}, topic string) (*pubsub.NewMessage, error) { data, err := json.Marshal(cloudEvent) if err != nil { return nil, err } return &pubsub.NewMessage{ Data: data, Topic: topic, Metadata: make(map[string]string), }, nil }Also applies to: 97-101
pkg/grpc/default_api/api_subscribe.go (2)
151-165: Unlock mutex before logging to prevent holding the lock during I/O operationsIn the
Subscribemethod of thestreamerstruct, the locks.lockis held while logging the subscription event. Holding a mutex during I/O operations like logging can lead to performance bottlenecks and reduce concurrency.You can modify the code to unlock the mutex before logging:
...
175-185: Return nil on normal stream closure to avoid unnecessary error propagationIn the
Subscribemethod, when the client stream ends normally (e.g., due toio.EOF), returning the error can cause unnecessary error handling upstream and may result in confusing logs.Apply the following change to return
nilon expected stream closures:...spec/proto/runtime/v1/runtime.proto (1)
814-816: Correct the comment to match the message nameThe comment references
SubscribeTopicEventsResponseInitialAlpha1, but the message is namedSubscribeTopicEventsResponseInitial. This could cause confusion.Apply this diff to correct the comment:
-// SubscribeTopicEventsResponseInitialAlpha1 is the initial response from layotto +// SubscribeTopicEventsResponseInitial is the initial response from layotto
🛑 Comments failed to post (10)
demo/pubsub/README.md (1)
21-35:
⚠️ Potential issueFix typo and improve consistency in SubscribeTopicEvents demo instructions.
There's a typo in the subscriber command and inconsistent indentation:
Apply this fix:
#################### Run pubsub demo with SubscribeTopicEvents #################### -# 1. start layotto +# 1. Start layotto (in terminal 1) cd ${project_path}/cmd/layotto go build -o layotto ./layotto start -c ../../configs/config_standalone.json cd ${project_path}/demo/pubsub/dynamic/ -# 2. start subscriber +# 2. Start subscriber (in terminal 2) go build -o subscriber -/.subscriber -s pub_subs_demo +./subscriber -s pub_subs_demo # pub_subs_demo is the store name -# 3. start publisher - cd ${project_path}/demo/pubsub/client/ +# 3. Start publisher (in terminal 3) +cd ${project_path}/demo/pubsub/client/ go build -o publisher - ./publisher -s pub_subs_demo +./publisher -s pub_subs_demo # Use the same store name as subscriber📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.#################### Run pubsub demo with SubscribeTopicEvents #################### # 1. Start layotto (in terminal 1) cd ${project_path}/cmd/layotto go build -o layotto ./layotto start -c ../../configs/config_standalone.json cd ${project_path}/demo/pubsub/dynamic/ # 2. Start subscriber (in terminal 2) go build -o subscriber ./subscriber -s pub_subs_demo # pub_subs_demo is the store name # 3. Start publisher (in terminal 3) cd ${project_path}/demo/pubsub/client/ go build -o publisher ./publisher -s pub_subs_demo # Use the same store name as subscriberdemo/pubsub/server/dynamic/subscribe_server.go (2)
49-54: 🛠️ Refactor suggestion
Add context with timeout for subscriptions
The subscription operations should have a timeout to prevent indefinite blocking:
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() - stop, err := cli.SubscribeWithHandler(context.Background(), client.SubscriptionRequest{ + stop, err := cli.SubscribeWithHandler(ctx, client.SubscriptionRequest{ PubsubName: storeName, Topic: "hello", Metadata: nil, }, eventHandler) //Use Subscribe API to subscribe to a topic. - sub, err := cli.Subscribe(context.Background(), client.SubscriptionRequest{ + sub, err := cli.Subscribe(ctx, client.SubscriptionRequest{Also applies to: 66-78
82-84:
⚠️ Potential issueFix incompatible errors.Join usage
The
errors.Joinfunction is not available in the current Go version. Consider using a different approach for error handling:- if err := errors.Join(stop(), sub.Close()); err != nil { - log.Fatal(err) - } + if err := stop(); err != nil { + log.Printf("Error stopping subscription: %v", err) + } + if err := sub.Close(); err != nil { + log.Printf("Error closing subscription: %v", err) + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if err := stop(); err != nil { log.Printf("Error stopping subscription: %v", err) } if err := sub.Close(); err != nil { log.Printf("Error closing subscription: %v", err) }🧰 Tools
🪛 GitHub Check: Go CI Linter (./demo)
[failure] 82-82:
Join not declared by package errors (typecheck)pkg/grpc/default_api/api_subscribe_test.go (1)
131-132: 🛠️ Refactor suggestion
Add assertions for message content
The test should verify that the published message contains the expected content.
err = apiForTest.publishMessageForStream(context.Background(), msg, pubsubName) assert.Nil(t, err) + // Verify message content + stream.EXPECT().Send(gomock.Any()).Do(func(req *runtimev1pb.SubscribeTopicEventsResponse) { + assert.Equal(t, msg.Topic, req.TopicEvent.Topic) + assert.Equal(t, msg.Data, req.TopicEvent.Data) + assert.Equal(t, msg.Metadata, req.TopicEvent.Metadata) + }).Return(nil)Committable suggestion skipped: line range outside the PR's diff.
pkg/grpc/default_api/api.go (2)
90-95:
⚠️ Potential issueImplement complete shutdown mechanism
The new fields suggest a graceful shutdown pattern, but the implementation appears incomplete. The
closedflag,closeChchannel, andwgWaitGroup need corresponding shutdown logic in aClose()method.Please implement a
Close()method that:
- Sets the atomic closed flag
- Closes the closeCh channel
- Waits for all goroutines to complete using wg
- Properly cleans up the streamer
Example implementation:
func (a *api) Close() error { if a.closed.Swap(true) { return nil // already closed } close(a.closeCh) a.wg.Wait() if a.streamer != nil { return a.streamer.close() } return nil }
90-90: 💡 Codebase verification
Based on the results, I can now generate the final response since I have sufficient information about the streamer implementation and synchronization primitives usage:
Initialization of streamer and sync primitives is required
The
streamerfield is used for managing pub/sub functionality and requires proper initialization:
- The
streamerstruct is defined with subscribers map and sync.RWMutex- WaitGroup (
wg) is actively used in goroutines for subscription handlingcloseChis used in select statements for graceful shutdownRequired initializations in constructor:
streamer: &streamer{subscribers: make(map[string]*conn)}, closeCh: make(chan struct{}), wg: &sync.WaitGroup{},🔗 Analysis chain
Verify streamer implementation and usage
The
streamerfield suggests pub/sub functionality, but we need to verify its implementation and usage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check streamer implementation echo "Checking streamer implementation..." ast-grep --pattern 'type streamer struct { $$$ }' # Check for goroutines using wg echo "Checking WaitGroup usage..." rg "wg\.(Add|Done)" -A 5 # Check for closeCh usage in select statements echo "Checking closeCh usage..." ast-grep --pattern 'select { case $_ <- $_.closeCh: $$$ }'Length of output: 23774
pkg/grpc/default_api/api_subscribe.go (2)
227-232: 🛠️ Refactor suggestion
Add timeout when waiting for client response to prevent indefinite blocking
In
publishMessageForStream, after sending a message to the client, the server waits indefinitely for the client's response. If the client fails to respond (e.g., due to a network issue or crash), this can lead to goroutine leaks and resource exhaustion.You can add a timeout to prevent indefinite blocking:
+ timeoutCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + case <-timeoutCtx.Done(): + return timeoutCtx.Err()Ensure that you import the
timepackage at the top of your file:import ( // existing imports "time" )
93-95: 🛠️ Refactor suggestion
Protect shared resource with mutex to ensure thread safety
The map
a.topicPerComponentis modified without synchronization. Since it can be accessed concurrently by multiple goroutines, this can lead to race conditions.Consider using a mutex to protect access to
a.topicPerComponent:+ a.topicLock.Lock() if a.topicPerComponent == nil { a.topicPerComponent = make(map[string]TopicSubscriptions) } + a.topicLock.Unlock()Don't forget to define the mutex in your
apistruct:type api struct { // existing fields + topicLock sync.Mutex }pkg/grpc/default_api/api_pubsub.go (2)
236-239:
⚠️ Potential issueReturn the correct error when base64 decoding fails
In
envelopeFromSubscriptionMessage, when base64 decoding fails, the function returns the wrong error variable. It should returndecodeErrinstead oferr.Apply this diff to fix the issue:
if decodeErr != nil { log.DefaultLogger.Debugf("unable to base64 decode cloudEvent field data_base64: %s", decodeErr) - return nil, cloudEvent, err + return nil, cloudEvent, decodeErr }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.decoded, decodeErr := base64.StdEncoding.DecodeString(data.(string)) if decodeErr != nil { log.DefaultLogger.Debugf("unable to base64 decode cloudEvent field data_base64: %s", decodeErr) return nil, cloudEvent, decodeErr
249-250:
⚠️ Potential issueHandle error from JSON marshalling when setting envelope data
In
envelopeFromSubscriptionMessage, the error returned bya.json.Marshal(data)is ignored. This could lead to unexpected behavior if marshalling fails.Apply this diff to handle the error:
if contenttype.IsJSONContentType(envelope.DataContentType) { - envelope.Data, _ = a.json.Marshal(data) + envelope.Data, err = a.json.Marshal(data) + if err != nil { + log.DefaultLogger.Debugf("unable to marshal data field: %s", err) + return nil, cloudEvent, err + } }Committable suggestion skipped: line range outside the PR's diff.
Signed-off-by: “huazhongming” <[email protected]>
CrazyHZM
force-pushed
the
feat/support_SubscribeTopicEvents
branch
from
b9ab1e7
to
530e88d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
spec/proto/runtime/v1/runtime.proto (2)
778-796: Consider adding validation constraints for topic namesThe
SubscribeTopicEventsRequestInitialmessage could benefit from field constraints:
- Consider adding field validation rules for
topicto enforce naming conventions- Document any restrictions on
dead_letter_topicnaming
820-822: Consider documenting empty message purposeThe empty message
SubscribeTopicEventsResponseInitialshould have documentation explaining why it's empty and its role in the protocol.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
spec/proto/runtime/v1/runtime.proto(3 hunks)
🔇 Additional comments (5)
spec/proto/runtime/v1/runtime.proto (5)
73-75: LGTM: Well-documented streaming RPC definition
The RPC definition for SubscribeTopicEvents is well-documented and correctly implements bi-directional streaming for pub/sub functionality.
764-776: LGTM: Well-structured request message with clear type discrimination
The SubscribeTopicEventsRequest message is well-designed:
- Uses
oneoffor type discrimination between initial and processed requests - Includes clear documentation
- Follows protobuf best practices
798-806: Verify TopicEventResponse type definition
The SubscribeTopicEventsRequestProcessed message uses TopicEventResponse which should be defined in the imported appcallback.proto.
#!/bin/bash
# Verify TopicEventResponse definition
fd -e proto -x grep -l "message TopicEventResponse" {}808-818: Verify TopicEventRequest type definition
The SubscribeTopicEventsResponse message uses TopicEventRequest which should be defined in the imported appcallback.proto.
#!/bin/bash
# Verify TopicEventRequest definition
fd -e proto -x grep -l "message TopicEventRequest" {}7-7: Verify the existence of imported proto file
The import statement references "spec/proto/runtime/v1/appcallback.proto", which needs to be verified to ensure it exists and contains the required message types (TopicEventResponse and TopicEventRequest).
✅ Verification successful
Import statement is valid and contains required message types
The imported file spec/proto/runtime/v1/appcallback.proto exists and contains both required message types:
TopicEventRequestwith all necessary fields for CloudEvents compatibilityTopicEventResponsewith status handling capabilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the imported proto file exists and contains required message types
fd -e proto -x cat {} \; | grep -A 5 "message \(TopicEventResponse\|TopicEventRequest\)"
# Verify the import path
fd "appcallback.proto$"
Length of output: 238
Script:
#!/bin/bash
# Check if the imported proto file exists
fd "appcallback.proto$"
# If file exists, check its contents
if fd "appcallback.proto$" -x cat {} \;; then
echo "=== Checking message definitions ==="
fd "appcallback.proto$" -x grep -A 5 "message \(TopicEventResponse\|TopicEventRequest\)" {}
fi
Length of output: 4170
demo/pubsub/README.md
Outdated
|
|
||
| #################### Run pubsub demo with SubscribeTopicEvents #################### | ||
| # 1. start layotto | ||
| cd ${project_path}/cmd/layotto |
There was a problem hiding this comment.
因为 CI 里会运行这些作为 demo 的 markdown 文件（执行这些 markdown 文件里的 shell 脚本），这样直接添加新的shell 脚本可能让 CI 跑不通哈，详细说明见 https://mosn.io/layotto/docs/development/test-quickstart
There was a problem hiding this comment.
目前看起来只能都合并完了，再加demo？
@seeflood
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (9)
pkg/grpc/default_api/api_subscribe_test.go (2)
66-66: Enhance error assertion specificityThe current error assertion only checks for the presence of an error. Consider asserting the specific error type or message to ensure the correct error is being returned.
-assert.Error(t, err, "Expected error") +expectedErr := "duplicate subscription" +assert.Error(t, err) +assert.Contains(t, err.Error(), expectedErr)
85-92: Extract cloud event setup to a helper functionConsider moving the cloud event setup to a helper function to improve test readability and reusability.
+func createTestCloudEvent(id string) map[string]interface{} { + return map[string]interface{}{ + pubsub.IDField: id, + pubsub.SourceField: "source", + pubsub.DataContentTypeField: "content-type", + pubsub.TypeField: "type", + pubsub.SpecVersionField: "v1.0.0", + pubsub.DataBase64Field: "bGF5b3R0bw==", + } +} func TestPublishMessageForStream(t *testing.T) { t.Run("publish success", func(t *testing.T) { // ... - cloudEvent := map[string]interface{}{ - pubsub.IDField: "1", - pubsub.SourceField: "source", - pubsub.DataContentTypeField: "content-type", - pubsub.TypeField: "type", - pubsub.SpecVersionField: "v1.0.0", - pubsub.DataBase64Field: "bGF5b3R0bw==", - } + cloudEvent := createTestCloudEvent("1")pkg/grpc/default_api/api.go (1)
Line range hint
44-45: Consider removing the global singleton patternThe
LayottoAPISingletonglobal variable introduces tight coupling and makes the code harder to test and maintain. The FIXME comment acknowledges this issue.Consider:
- Dependency injection instead of global state
- Context-based API instance management
- Factory pattern for API instance creation
Would you like assistance in designing a more maintainable alternative to the singleton pattern?
pkg/grpc/default_api/api_subscribe.go (4)
85-91: Consider trimming whitespace for input validationTo prevent subscriptions with empty or whitespace-only
pubsubNameortopic, consider trimming whitespace before checking the length.Apply this diff to trim whitespace:
if len(strings.TrimSpace(initialRequest.GetPubsubName())) == 0 { return errors.New("pubsubName is required") } if len(strings.TrimSpace(initialRequest.GetTopic())) == 0 { return errors.New("topic is required") }
133-145: Ensure proper cleanup on subscription errorsIf an error occurs during
Subscribe, consider cleaning up any state that was partially set up to prevent stale entries intopicPerComponent.Implement cleanup logic if
a.pubSubs[initialRequest.PubsubName].Subscribe()returns an error.
150-202: Handle context cancellations gracefullyWhen the client stream is canceled, returning an error may not be necessary since it's an expected behavior.
Modify the code to return
nilinstead of an error when the context is canceled:if (ok && s.Code() == codes.Canceled) || errors.Is(err, context.Canceled) || errors.Is(err, io.EOF) { log.DefaultLogger.Infof("Unsubscribed from pubsub '%s' topic '%s'", req.GetPubsubName(), req.GetTopic()) - return err + return nil }
265-267: Improve key generation to avoid potential collisionsThe
StreamerKeyfunction concatenates strings to create a key. Consider using a more robust method to avoid potential key collisions ifpubsubortopiccontain the separators used.Use a structured approach or encoding to generate the key:
-import "encoding/json" func (s *streamer) StreamerKey(pubsub, topic string) string { - return "___" + pubsub + "||" + topic + keyStruct := struct { + Pubsub string `json:"pubsub"` + Topic string `json:"topic"` + }{ + Pubsub: pubsub, + Topic: topic, + } + keyBytes, _ := json.Marshal(keyStruct) + return string(keyBytes) }spec/proto/runtime/v1/runtime.proto (2)
769-770: Misplaced comment may cause confusionThe comment
// The unique identifier for the subscription request.is placed above theoneofdeclaration. It would be clearer if this comment is moved to line 802, above theidfield inSubscribeTopicEventsRequestProcessed, whereidis actually defined.
820-822: Inconsistent naming between comment and messageThe comment mentions
SubscribeTopicEventsResponseInitialAlpha1, but the actual message is namedSubscribeTopicEventsResponseInitial. Ensure the comment matches the message name for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
pkg/mock/runtime/runtime.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
demo/pubsub/README.md(1 hunks)pkg/grpc/default_api/api.go(3 hunks)pkg/grpc/default_api/api_pubsub.go(2 hunks)pkg/grpc/default_api/api_subscribe.go(1 hunks)pkg/grpc/default_api/api_subscribe_test.go(1 hunks)spec/proto/runtime/v1/runtime.proto(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- demo/pubsub/README.md
🧰 Additional context used
🪛 GitHub Check: Go CI Linter (.)
pkg/grpc/default_api/api.go
[failure] 93-93:
Bool not declared by package atomic (typecheck)
🔇 Additional comments (7)
pkg/grpc/default_api/api.go (1)
23-23: LGTM: Import addition is appropriate
The sync/atomic import is correctly added to support the new atomic.Bool field.
pkg/grpc/default_api/api_subscribe.go (3)
49-69: Overall implementation looks solid
The SubscribeTopicEvents function is well-structured, handling the stream subscription and server shutdown gracefully using goroutines and channels.
125-131: Handle error when sending the initial response to the client
After sending the initial response with stream.Send(), you're already checking the error. Great job!
213-214:
Handle error from envelopeFromSubscriptionMessage
The error returned by a.envelopeFromSubscriptionMessage(ctx, msg) is not checked. This could lead to unhandled errors and unexpected behavior.
Apply this diff to handle the error:
envelope, cloudEvent, err := a.envelopeFromSubscriptionMessage(ctx, msg)
+if err != nil {
+ return err
+}Likely invalid or redundant comment.
spec/proto/runtime/v1/runtime.proto (3)
801-806: Undefined type 'TopicEventResponse' used in 'SubscribeTopicEventsRequestProcessed'
The field status in SubscribeTopicEventsRequestProcessed is of type TopicEventResponse, which is not defined in this file or properly imported. This will lead to compilation errors.
816-816: Undefined type 'TopicEventRequest' used in 'SubscribeTopicEventsResponse'
The field event_message in SubscribeTopicEventsResponse is of type TopicEventRequest, which is not defined in this file or properly imported. This will cause compilation errors.
7-7: Import of 'appcallback.proto' may be incorrect or missing
The import statement references "spec/proto/runtime/v1/appcallback.proto", but previous issues indicate that this file may not exist or is incorrectly referenced. Verify that appcallback.proto exists at the specified path and adjust the import if necessary to prevent compilation errors.
| err = apiForTest.publishMessageForStream(context.Background(), msg, pubsubName) | ||
| assert.Nil(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions for published message content
The test should verify that the message was published with the correct content and metadata.
err = apiForTest.publishMessageForStream(context.Background(), msg, pubsubName)
assert.Nil(t, err)
+ // Verify the published message
+ assert.Equal(t, topic, msg.Topic)
+ var publishedEvent map[string]interface{}
+ err = json.Unmarshal(msg.Data, &publishedEvent)
+ assert.Nil(t, err)
+ assert.Equal(t, "1", publishedEvent[pubsub.IDField])
+ assert.Equal(t, "source", publishedEvent[pubsub.SourceField])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = apiForTest.publishMessageForStream(context.Background(), msg, pubsubName) | |
| assert.Nil(t, err) | |
| err = apiForTest.publishMessageForStream(context.Background(), msg, pubsubName) | |
| assert.Nil(t, err) | |
| // Verify the published message | |
| assert.Equal(t, topic, msg.Topic) | |
| var publishedEvent map[string]interface{} | |
| err = json.Unmarshal(msg.Data, &publishedEvent) | |
| assert.Nil(t, err) | |
| assert.Equal(t, "1", publishedEvent[pubsub.IDField]) | |
| assert.Equal(t, "source", publishedEvent[pubsub.SourceField]) |
pkg/grpc/default_api/api.go
Outdated
| streamer *streamer | ||
| // json | ||
| json jsoniter.API | ||
| json jsoniter.API | ||
| closed atomic.Bool | ||
| closeCh chan struct{} | ||
| wg sync.WaitGroup |
There was a problem hiding this comment.
Fix atomic.Bool type and ensure proper field initialization
The atomic.Bool type usage is incorrect. Additionally, these concurrency-related fields need proper initialization in the constructor.
- Fix the atomic.Bool type:
- closed atomic.Bool
+ closed atomic.Value // For Go < 1.19
// OR
+ closed atomic.Bool{} // For Go >= 1.19- Ensure all fields are properly initialized in NewAPI:
return &api{
// ... existing fields ...
json: jsoniter.ConfigFastest,
+ streamer: newStreamer(),
+ closeCh: make(chan struct{}),
+ closed: atomic.Bool{}, // or atomic.Value for Go < 1.19
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Go CI Linter (.)
[failure] 93-93:
Bool not declared by package atomic (typecheck)
| decoded, decodeErr := base64.StdEncoding.DecodeString(data.(string)) | ||
| if decodeErr != nil { | ||
| log.DefaultLogger.Debugf("unable to base64 decode cloudEvent field data_base64: %s", decodeErr) | ||
| return nil, cloudEvent, err | ||
| } |
There was a problem hiding this comment.
Fix error variable in base64 decode error handling.
The error handling for base64 decode returns the wrong error variable.
Apply this diff to fix the error handling:
decoded, decodeErr := base64.StdEncoding.DecodeString(data.(string))
if decodeErr != nil {
log.DefaultLogger.Debugf("unable to base64 decode cloudEvent field data_base64: %s", decodeErr)
- return nil, cloudEvent, err
+ return nil, cloudEvent, decodeErr
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| decoded, decodeErr := base64.StdEncoding.DecodeString(data.(string)) | |
| if decodeErr != nil { | |
| log.DefaultLogger.Debugf("unable to base64 decode cloudEvent field data_base64: %s", decodeErr) | |
| return nil, cloudEvent, err | |
| } | |
| decoded, decodeErr := base64.StdEncoding.DecodeString(data.(string)) | |
| if decodeErr != nil { | |
| log.DefaultLogger.Debugf("unable to base64 decode cloudEvent field data_base64: %s", decodeErr) | |
| return nil, cloudEvent, decodeErr | |
| } |
| if contenttype.IsStringContentType(envelope.DataContentType) { | ||
| envelope.Data = []byte(data.(string)) | ||
| } else if contenttype.IsJSONContentType(envelope.DataContentType) { | ||
| envelope.Data, _ = a.json.Marshal(data) | ||
| } |
There was a problem hiding this comment.
Add safe type assertions and handle JSON marshal errors.
The current implementation has unsafe type assertions for string data and ignores JSON marshal errors.
Apply this diff to add safe type assertions and handle marshal errors:
if contenttype.IsStringContentType(envelope.DataContentType) {
- envelope.Data = []byte(data.(string))
+ dataStr, ok := data.(string)
+ if !ok {
+ return nil, cloudEvent, fmt.Errorf("invalid type for data field: expected string")
+ }
+ envelope.Data = []byte(dataStr)
} else if contenttype.IsJSONContentType(envelope.DataContentType) {
- envelope.Data, _ = a.json.Marshal(data)
+ var err error
+ envelope.Data, err = a.json.Marshal(data)
+ if err != nil {
+ return nil, cloudEvent, fmt.Errorf("failed to marshal data field: %v", err)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if contenttype.IsStringContentType(envelope.DataContentType) { | |
| envelope.Data = []byte(data.(string)) | |
| } else if contenttype.IsJSONContentType(envelope.DataContentType) { | |
| envelope.Data, _ = a.json.Marshal(data) | |
| } | |
| if contenttype.IsStringContentType(envelope.DataContentType) { | |
| dataStr, ok := data.(string) | |
| if !ok { | |
| return nil, cloudEvent, fmt.Errorf("invalid type for data field: expected string") | |
| } | |
| envelope.Data = []byte(dataStr) | |
| } else if contenttype.IsJSONContentType(envelope.DataContentType) { | |
| var err error | |
| envelope.Data, err = a.json.Marshal(data) | |
| if err != nil { | |
| return nil, cloudEvent, fmt.Errorf("failed to marshal data field: %v", err) | |
| } | |
| } |
pkg/grpc/default_api/api_pubsub.go
Outdated
| envelope, cloudEvent, err := a.envelopeFromSubscriptionMessage(ctx, msg) | ||
| // Call appcallback | ||
| clientV1 := runtimev1pb.NewAppCallbackClient(a.AppCallbackConn) | ||
| res, err := clientV1.OnTopicEvent(ctx, envelope) | ||
|
|
||
| // 5. Check result | ||
| // Check result |
There was a problem hiding this comment.
Add error handling and nil checks.
The current implementation has two potential issues:
- The error from
envelopeFromSubscriptionMessageis not checked - Missing nil check for
envelopewhich could be nil for expired messages
Apply this diff to fix both issues:
envelope, cloudEvent, err := a.envelopeFromSubscriptionMessage(ctx, msg)
+if err != nil {
+ return err
+}
+if envelope == nil {
+ // Message has expired, skip processing
+ return nil
+}
// Call appcallback
clientV1 := runtimev1pb.NewAppCallbackClient(a.AppCallbackConn)
res, err := clientV1.OnTopicEvent(ctx, envelope)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| envelope, cloudEvent, err := a.envelopeFromSubscriptionMessage(ctx, msg) | |
| // Call appcallback | |
| clientV1 := runtimev1pb.NewAppCallbackClient(a.AppCallbackConn) | |
| res, err := clientV1.OnTopicEvent(ctx, envelope) | |
| // 5. Check result | |
| // Check result | |
| envelope, cloudEvent, err := a.envelopeFromSubscriptionMessage(ctx, msg) | |
| if err != nil { | |
| return err | |
| } | |
| if envelope == nil { | |
| // Message has expired, skip processing | |
| return nil | |
| } | |
| // Call appcallback | |
| clientV1 := runtimev1pb.NewAppCallbackClient(a.AppCallbackConn) | |
| res, err := clientV1.OnTopicEvent(ctx, envelope) | |
| // Check result |
| Id: cloudEvent[pubsub.IDField].(string), | ||
| Source: cloudEvent[pubsub.SourceField].(string), | ||
| DataContentType: cloudEvent[pubsub.DataContentTypeField].(string), | ||
| Type: cloudEvent[pubsub.TypeField].(string), | ||
| SpecVersion: cloudEvent[pubsub.SpecVersionField].(string), | ||
| Topic: msg.Topic, | ||
| PubsubName: msg.Metadata[Metadata_key_pubsubName], | ||
| } |
There was a problem hiding this comment.
Add validation for required cloud event fields and safe type assertions.
The current implementation performs unsafe type assertions which could cause panics. Additionally, there's no validation for required cloud event fields.
Apply this diff to add validation and safe type assertions:
+ // Validate required fields
+ requiredFields := []string{pubsub.IDField, pubsub.SourceField, pubsub.DataContentTypeField, pubsub.TypeField, pubsub.SpecVersionField}
+ for _, field := range requiredFields {
+ if _, ok := cloudEvent[field]; !ok {
+ return nil, cloudEvent, fmt.Errorf("required field %s missing from cloud event", field)
+ }
+ }
+
+ // Safe type assertions
+ id, ok := cloudEvent[pubsub.IDField].(string)
+ if !ok {
+ return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.IDField)
+ }
+ source, ok := cloudEvent[pubsub.SourceField].(string)
+ if !ok {
+ return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.SourceField)
+ }
+ contentType, ok := cloudEvent[pubsub.DataContentTypeField].(string)
+ if !ok {
+ return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.DataContentTypeField)
+ }
+ eventType, ok := cloudEvent[pubsub.TypeField].(string)
+ if !ok {
+ return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.TypeField)
+ }
+ specVersion, ok := cloudEvent[pubsub.SpecVersionField].(string)
+ if !ok {
+ return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.SpecVersionField)
+ }
envelope := &runtimev1pb.TopicEventRequest{
- Id: cloudEvent[pubsub.IDField].(string),
- Source: cloudEvent[pubsub.SourceField].(string),
- DataContentType: cloudEvent[pubsub.DataContentTypeField].(string),
- Type: cloudEvent[pubsub.TypeField].(string),
- SpecVersion: cloudEvent[pubsub.SpecVersionField].(string),
+ Id: id,
+ Source: source,
+ DataContentType: contentType,
+ Type: eventType,
+ SpecVersion: specVersion,
Topic: msg.Topic,
PubsubName: msg.Metadata[Metadata_key_pubsubName],
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Id: cloudEvent[pubsub.IDField].(string), | |
| Source: cloudEvent[pubsub.SourceField].(string), | |
| DataContentType: cloudEvent[pubsub.DataContentTypeField].(string), | |
| Type: cloudEvent[pubsub.TypeField].(string), | |
| SpecVersion: cloudEvent[pubsub.SpecVersionField].(string), | |
| Topic: msg.Topic, | |
| PubsubName: msg.Metadata[Metadata_key_pubsubName], | |
| } | |
| // Validate required fields | |
| requiredFields := []string{pubsub.IDField, pubsub.SourceField, pubsub.DataContentTypeField, pubsub.TypeField, pubsub.SpecVersionField} | |
| for _, field := range requiredFields { | |
| if _, ok := cloudEvent[field]; !ok { | |
| return nil, cloudEvent, fmt.Errorf("required field %s missing from cloud event", field) | |
| } | |
| } | |
| // Safe type assertions | |
| id, ok := cloudEvent[pubsub.IDField].(string) | |
| if !ok { | |
| return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.IDField) | |
| } | |
| source, ok := cloudEvent[pubsub.SourceField].(string) | |
| if !ok { | |
| return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.SourceField) | |
| } | |
| contentType, ok := cloudEvent[pubsub.DataContentTypeField].(string) | |
| if !ok { | |
| return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.DataContentTypeField) | |
| } | |
| eventType, ok := cloudEvent[pubsub.TypeField].(string) | |
| if !ok { | |
| return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.TypeField) | |
| } | |
| specVersion, ok := cloudEvent[pubsub.SpecVersionField].(string) | |
| if !ok { | |
| return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.SpecVersionField) | |
| } | |
| envelope := &runtimev1pb.TopicEventRequest{ | |
| Id: id, | |
| Source: source, | |
| DataContentType: contentType, | |
| Type: eventType, | |
| SpecVersion: specVersion, | |
| Topic: msg.Topic, | |
| PubsubName: msg.Metadata[Metadata_key_pubsubName], | |
| } |
| return retryStrategy(err, resp.Status, cloudEvent) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure retryStrategy receives the correct error
Since err might have been overwritten, ensure that the correct error is passed to retryStrategy.
Modify the return statement:
-return retryStrategy(err, resp.Status, cloudEvent)
+return retryStrategy(nil, resp.Status, cloudEvent)Committable suggestion skipped: line range outside the PR's diff.
| err = conn.stream.Send(&runtimev1pb.SubscribeTopicEventsResponse{ | ||
| SubscribeTopicEventsResponseType: &runtimev1pb.SubscribeTopicEventsResponse_EventMessage{ | ||
| EventMessage: envelope, | ||
| }, | ||
| }) | ||
| conn.streamLock.Unlock() |
There was a problem hiding this comment.
Check error when sending message to the client
After sending the event message with conn.stream.Send(), the returned error err should be checked to handle any issues during the send operation.
Apply this diff to handle the error:
conn.streamLock.Lock()
err = conn.stream.Send(&runtimev1pb.SubscribeTopicEventsResponse{
SubscribeTopicEventsResponseType: &runtimev1pb.SubscribeTopicEventsResponse_EventMessage{
EventMessage: envelope,
},
})
conn.streamLock.Unlock()
+if err != nil {
+ return err
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = conn.stream.Send(&runtimev1pb.SubscribeTopicEventsResponse{ | |
| SubscribeTopicEventsResponseType: &runtimev1pb.SubscribeTopicEventsResponse_EventMessage{ | |
| EventMessage: envelope, | |
| }, | |
| }) | |
| conn.streamLock.Unlock() | |
| err = conn.stream.Send(&runtimev1pb.SubscribeTopicEventsResponse{ | |
| SubscribeTopicEventsResponseType: &runtimev1pb.SubscribeTopicEventsResponse_EventMessage{ | |
| EventMessage: envelope, | |
| }, | |
| }) | |
| conn.streamLock.Unlock() | |
| if err != nil { | |
| return err | |
| } |
CrazyHZM
force-pushed
the
feat/support_SubscribeTopicEvents
branch
from
d6e8a3b
to
4d66ccd
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
pkg/grpc/default_api/api_subscribe_test.go (1)
35-68: Add cleanup and improve test structure.While the past review already suggested adding more test scenarios, there are additional improvements needed:
- Add cleanup of resources in the test using
t.Cleanup()to ensure proper teardown- Consider using test table pattern for better test organization when adding the suggested scenarios
func TestSubscribeTopicEvents(t *testing.T) { + t.Cleanup(func() { + // Cleanup resources + }) + + tests := []struct { + name string + setup func(*testing.T) (*api, *runtime.MockRuntime_SubscribeTopicEventsServer) + wantErr bool + }{ + { + name: "duplicate initial request returns error", + setup: func(t *testing.T) (*api, *runtime.MockRuntime_SubscribeTopicEventsServer) { // existing test setup code + }, + wantErr: true, + }, + // Add more test cases here + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + api, stream := tt.setup(t) + err := api.SubscribeTopicEvents(stream) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } }pkg/grpc/default_api/api.go (2)
88-88: Add documentation for the streamer field.Please add a comment explaining the purpose and responsibility of the
streamerfield to improve code maintainability.
88-92: Consider documenting the concurrent design pattern.The addition of
streamer,closeCh, andwgsuggests implementation of a concurrent subscription system. Consider adding package-level documentation explaining:
- The overall concurrent design pattern
- How these fields interact in the subscription lifecycle
- The shutdown sequence and cleanup process
This will help future maintainers understand the system's architecture and threading model.
spec/proto/runtime/v1/runtime.proto (2)
770-774: Clarify the comment for 'event_processed' fieldThe comment for
event_processedreads, "The message containing the subscription to a topic." To improve clarity, consider updating it to, "The message indicating the result of processing an event," to accurately reflect its purpose.
820-822: Correct the comment to match the message nameThe comment refers to
SubscribeTopicEventsResponseInitialAlpha1, but the message is namedSubscribeTopicEventsResponseInitial. Please update the comment for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
pkg/mock/runtime/runtime.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
demo/pubsub/README.md(1 hunks)pkg/grpc/default_api/api.go(3 hunks)pkg/grpc/default_api/api_pubsub.go(2 hunks)pkg/grpc/default_api/api_subscribe.go(1 hunks)pkg/grpc/default_api/api_subscribe_test.go(1 hunks)spec/proto/runtime/v1/runtime.proto(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- demo/pubsub/README.md
🧰 Additional context used
🪛 GitHub Check: Go CI Linter (.)
pkg/grpc/default_api/api_pubsub.go
[failure] 156-156:
ineffectual assignment to err (ineffassign)
pkg/grpc/default_api/api_subscribe.go
[failure] 213-213:
ineffectual assignment to err (ineffassign)
pkg/grpc/default_api/api_subscribe_test.go
[failure] 122-122:
S1005: unnecessary assignment to the blank identifier (gosimple)
🔇 Additional comments (8)
pkg/grpc/default_api/api_subscribe.go (3)
213-213: Handle error returned by envelopeFromSubscriptionMessage
The error returned by a.envelopeFromSubscriptionMessage(ctx, msg) is not checked. Ignoring this error could lead to unintended behavior or panics if envelope or cloudEvent are invalid.
🧰 Tools
🪛 GitHub Check: Go CI Linter (.)
[failure] 213-213:
ineffectual assignment to err (ineffassign)
219-224: Check error when sending message to the client
After sending the event message with conn.stream.Send(), the returned error err should be checked to handle any issues during the send operation. Ignoring this error may result in silent failures or missed messages.
234-235: Ensure retryStrategy receives the correct error value
The err variable might not reflect any errors from previous operations due to being overwritten or not updated. Passing an incorrect error to retryStrategy can affect the retry logic and error handling.
pkg/grpc/default_api/api_pubsub.go (4)
156-161: Previous review comment still applies: Add error handling and nil checks.
🧰 Tools
🪛 GitHub Check: Go CI Linter (.)
[failure] 156-156:
ineffectual assignment to err (ineffassign)
225-232: Previous review comment still applies: Add validation for required cloud event fields and safe type assertions.
236-240: Previous review comment still applies: Fix error variable in base64 decode error handling.
246-250: Previous review comment still applies: Add safe type assertions and handle JSON marshal errors.
spec/proto/runtime/v1/runtime.proto (1)
75-75: Method 'SubscribeTopicEvents' definition appears correct
The method is appropriately defined to support bi-directional streaming for topic subscriptions.
| func TestPublishMessageForStream(t *testing.T) { | ||
| t.Run("publish success", func(t *testing.T) { | ||
| pubsubName := "test" | ||
| topic := "layotto" | ||
| ctrl := gomock.NewController(t) | ||
| stream := runtime.NewMockRuntime_SubscribeTopicEventsServer(ctrl) | ||
| stream.EXPECT().Send(gomock.Any()).Return(nil).Times(1) | ||
| stream.EXPECT().Recv().Return(&runtimev1pb.SubscribeTopicEventsRequest{ | ||
| SubscribeTopicEventsRequestType: &runtimev1pb.SubscribeTopicEventsRequest_InitialRequest{ | ||
| InitialRequest: &runtimev1pb.SubscribeTopicEventsRequestInitial{ | ||
| PubsubName: pubsubName, Topic: topic, | ||
| Metadata: make(map[string]string), | ||
| }, | ||
| }}, nil).Times(1) | ||
|
|
||
| cloudEvent := map[string]interface{}{ | ||
| pubsub.IDField: "1", | ||
| pubsub.SourceField: "source", | ||
| pubsub.DataContentTypeField: "content-type", | ||
| pubsub.TypeField: "type", | ||
| pubsub.SpecVersionField: "v1.0.0", | ||
| pubsub.DataBase64Field: "bGF5b3R0bw==", | ||
| } | ||
|
|
||
| data, err := json.Marshal(cloudEvent) | ||
| assert.Nil(t, err) | ||
|
|
||
| msg := &pubsub.NewMessage{ | ||
| Data: data, | ||
| Topic: topic, | ||
| Metadata: make(map[string]string), | ||
| } | ||
| a := NewAPI("", nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) | ||
|
|
||
| var apiForTest = a.(*api) | ||
|
|
||
| apiForTest.streamer = &streamer{ | ||
| subscribers: make(map[string]*conn), | ||
| } | ||
|
|
||
| apiForTest.streamer.subscribers["___test||layotto"] = &conn{ | ||
| stream: stream, | ||
| publishResponses: make(map[string]chan *runtimev1pb.SubscribeTopicEventsRequestProcessed), | ||
| } | ||
|
|
||
| err = apiForTest.SubscribeTopicEvents(stream) | ||
| assert.Nil(t, err) | ||
|
|
||
| apiForTest.json = jsoniter.ConfigFastest | ||
|
|
||
| go func() { | ||
| time.Sleep(1 * time.Second) | ||
| ch, _ := apiForTest.streamer.subscribers["___test||layotto"].publishResponses["1"] | ||
| ch <- &runtimev1pb.SubscribeTopicEventsRequestProcessed{ | ||
| Id: "1", | ||
| Status: &runtimev1pb.TopicEventResponse{ | ||
| Status: runtimev1pb.TopicEventResponse_SUCCESS, | ||
| }, | ||
| } | ||
| }() | ||
|
|
||
| err = apiForTest.publishMessageForStream(context.Background(), msg, pubsubName) | ||
| assert.Nil(t, err) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error test cases and fix flaky test.
While previous reviews addressed the time.Sleep and message content validation, there are additional improvements needed:
- Add test cases for error scenarios:
- Network timeout
- Invalid message format
- Missing required fields
- Fix the unnecessary blank identifier assignment on line 122
func TestPublishMessageForStream(t *testing.T) {
+ tests := []struct {
+ name string
+ msg *pubsub.NewMessage
+ setup func(*testing.T) *api
+ wantErr bool
+ }{
+ {
+ name: "publish success",
+ msg: createValidMessage(),
+ setup: func(t *testing.T) *api {
+ // Move existing setup here
+ },
+ wantErr: false,
+ },
+ {
+ name: "publish timeout",
+ msg: createValidMessage(),
+ setup: func(t *testing.T) *api {
+ // Setup that simulates timeout
+ },
+ wantErr: true,
+ },
+ // Add more error cases
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ api := tt.setup(t)
+ err := api.publishMessageForStream(context.Background(), tt.msg, "test")
+ if tt.wantErr {
+ assert.Error(t, err)
+ } else {
+ assert.NoError(t, err)
+ }
+ })
+ }
}
- ch, _ := apiForTest.streamer.subscribers["___test||layotto"].publishResponses["1"]
+ ch := apiForTest.streamer.subscribers["___test||layotto"].publishResponses["1"]Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Go CI Linter (.)
[failure] 122-122:
S1005: unnecessary assignment to the blank identifier (gosimple)
| if a.topicPerComponent == nil { | ||
| a.topicPerComponent = make(map[string]TopicSubscriptions) | ||
| } | ||
|
|
||
| if a.streamer == nil { | ||
| a.streamer = &streamer{ | ||
| subscribers: make(map[string]*conn), | ||
| } | ||
| } |
There was a problem hiding this comment.
Protect shared variables with synchronization mechanisms
The shared variables a.topicPerComponent and a.streamer are accessed and modified without any synchronization. If streamSubscribe can be called concurrently, this may lead to data races and unpredictable behavior.
To ensure thread safety, consider using mutexes to protect access to these shared variables. For example, you can introduce a mutex in the api struct and lock it when accessing or modifying a.topicPerComponent and a.streamer:
type api struct {
// ...
topicPerComponentLock sync.Mutex
streamerLock sync.Mutex
// ...
}Then, adjust the code to lock and unlock appropriately:
func (a *api) streamSubscribe(stream runtimev1pb.Runtime_SubscribeTopicEventsServer, subDone chan struct{}) error {
+ a.topicPerComponentLock.Lock()
if a.topicPerComponent == nil {
a.topicPerComponent = make(map[string]TopicSubscriptions)
}
+ a.topicPerComponentLock.Unlock()
+ a.streamerLock.Lock()
if a.streamer == nil {
a.streamer = &streamer{
subscribers: make(map[string]*conn),
}
}
+ a.streamerLock.Unlock()
// ...
}| // TODO tracing | ||
|
|
||
| // 4. Call appcallback | ||
| envelope, cloudEvent, err := a.envelopeFromSubscriptionMessage(ctx, msg) |
There was a problem hiding this comment.
Fix ineffectual assignment to err variable.
The variable err assigned from envelopeFromSubscriptionMessage is not used, which may lead to ignoring potential errors. Consider handling the error or explicitly ignoring it.
Apply the following fix to handle the error:
envelope, cloudEvent, err := a.envelopeFromSubscriptionMessage(ctx, msg)
+if err != nil {
+ return err
+}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Go CI Linter (.)
[failure] 156-156:
ineffectual assignment to err (ineffassign)
| // dead_letter_topic is the topic to which messages that fail to be processed | ||
| // are sent. | ||
| optional string dead_letter_topic = 4; | ||
| } |
There was a problem hiding this comment.
Ensure compatibility of 'optional' keyword in 'dead_letter_topic'
The use of the optional keyword for dead_letter_topic requires Protobuf compiler version 3.12 or later. Using an older compiler may lead to compilation errors. Ensure that your development environment uses a compatible version.
CrazyHZM
force-pushed
the
feat/support_SubscribeTopicEvents
branch
from
4d66ccd
to
b486e46
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (4)
pkg/grpc/default_api/api.go (1)
89-93: Group and document related fieldsConsider grouping these concurrency-related fields together and adding documentation to explain their purpose:
type api struct { // ... existing fields ... + // Subscription management + streamer *streamer + closeCh chan struct{} + wg sync.WaitGroup + + // Serialization + json jsoniter.API - streamer *streamer - // json - json jsoniter.API - closeCh chan struct{} - wg sync.WaitGroup }pkg/grpc/default_api/api_subscribe.go (1)
85-91: Trim whitespace when validating input parametersWhen checking if
pubsubNameandtopicare provided, consider trimming whitespace to prevent accepting inputs that are empty or contain only whitespace characters.Apply this diff to trim whitespace:
+import "strings" if len(strings.TrimSpace(initialRequest.GetPubsubName())) == 0 { return errors.New("pubsubName is required") } if len(strings.TrimSpace(initialRequest.GetTopic())) == 0 { return errors.New("topic is required") }spec/proto/runtime/v1/runtime.proto (2)
793-796: Check 'optional' keyword compatibility for 'dead_letter_topic'The use of the
optionalkeyword fordead_letter_topicmay require Protobuf compiler version 3.12 or later. Ensure that your build environment uses a compatible version to prevent any compilation issues.
820-822: Update comment to match the message nameThe comment on line 820 refers to
SubscribeTopicEventsResponseInitialAlpha1, but the actual message is defined asSubscribeTopicEventsResponseInitial. Please update the comment to reflect the correct message name.Apply the following diff to correct the comment:
-// SubscribeTopicEventsResponseInitialAlpha1 is the initial response from layotto +// SubscribeTopicEventsResponseInitial is the initial response from layotto
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
pkg/mock/runtime/runtime.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
demo/pubsub/README.md(1 hunks)pkg/grpc/default_api/api.go(2 hunks)pkg/grpc/default_api/api_pubsub.go(2 hunks)pkg/grpc/default_api/api_subscribe.go(1 hunks)pkg/grpc/default_api/api_subscribe_test.go(1 hunks)spec/proto/runtime/v1/runtime.proto(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- demo/pubsub/README.md
🔇 Additional comments (9)
pkg/grpc/default_api/api_subscribe_test.go (1)
131-132: 🛠️ Refactor suggestion
Add assertions for message content validation
The test should verify that the published message contains the expected content and metadata.
err = apiForTest.publishMessageForStream(context.Background(), msg, pubsubName)
assert.Nil(t, err)
+
+ // Verify message content
+ assert.Equal(t, topic, msg.Topic)
+ var publishedEvent map[string]interface{}
+ err = json.Unmarshal(msg.Data, &publishedEvent)
+ assert.Nil(t, err)
+ assert.Equal(t, "1", publishedEvent[pubsub.IDField])
+ assert.Equal(t, "source", publishedEvent[pubsub.SourceField])
+ assert.Equal(t, "content-type", publishedEvent[pubsub.DataContentTypeField])
+ assert.Equal(t, "type", publishedEvent[pubsub.TypeField])
+ assert.Equal(t, "v1.0.0", publishedEvent[pubsub.SpecVersionField])
+ assert.Equal(t, "bGF5b3R0bw==", publishedEvent[pubsub.DataBase64Field])Likely invalid or redundant comment.
pkg/grpc/default_api/api_subscribe.go (3)
93-101:
Protect shared resources with synchronization mechanisms
The shared variables a.topicPerComponent and a.streamer are accessed and modified without synchronization. Since streamSubscribe can be called concurrently, this may lead to data races. Use mutex locks to protect access to these shared resources.
Refer to the previous review comment for detailed guidance on adding synchronization.
219-225:
Handle the error returned by conn.stream.Send
After sending the event message to the client with conn.stream.Send, the returned error err should be checked to handle any issues during the send operation.
Refer to the previous review comment to handle the error appropriately.
233-234: 🛠️ Refactor suggestion
Pass the correct error to retryStrategy
The variable err may hold the error from conn.stream.Send, but if the send operation was successful, err would be nil, and any error from the context or response should be considered. Ensure that retryStrategy receives the appropriate error value.
Modify the return statement:
// 5. Check result
-return retryStrategy(err, resp.Status, cloudEvent)
+return retryStrategy(nil, resp.Status, cloudEvent)Likely invalid or redundant comment.
pkg/grpc/default_api/api_pubsub.go (2)
156-163: Refactored publishMessageGRPC for improved readability
The refactoring to use envelopeFromSubscriptionMessage enhances the clarity and maintainability of the publishMessageGRPC function. Consolidating the envelope creation logic reduces code duplication and simplifies error handling.
208-253: Introduction of envelopeFromSubscriptionMessage increases modularity
The new envelopeFromSubscriptionMessage method encapsulates the logic for unmarshalling the message and constructing the envelope. This promotes code reuse and adheres to the single-responsibility principle, making future maintenance easier.
spec/proto/runtime/v1/runtime.proto (3)
801-806: Undefined type 'TopicEventResponse' referenced
The type TopicEventResponse used in SubscribeTopicEventsRequestProcessed is not defined in this file. Ensure that TopicEventResponse is properly defined or imported from appcallback.proto.
816-816: Undefined type 'TopicEventRequest' referenced
The field event_message in SubscribeTopicEventsResponse uses the type TopicEventRequest, which is not defined in this file. Please make sure TopicEventRequest is defined or correctly imported.
7-7: Verify the import of 'appcallback.proto'
Ensure that 'spec/proto/runtime/v1/appcallback.proto' exists and is correctly imported. This file should define TopicEventRequest and TopicEventResponse used later in this proto file.
You can check the existence of appcallback.proto with the following script:
✅ Verification successful
Let me verify the content of the appcallback.proto file to ensure it defines the required messages.
Import of 'appcallback.proto' is correctly configured
The verification confirms that:
- The file exists at the correct path
- It properly defines both
TopicEventRequestandTopicEventResponsemessages that are required
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that 'appcallback.proto' exists in the specified directory
if [ -f "spec/proto/runtime/v1/appcallback.proto" ]; then
echo "appcallback.proto exists in the correct path."
else
echo "appcallback.proto does not exist or is misplaced."
fi
Length of output: 153
Script:
#!/bin/bash
# Check the content of appcallback.proto for TopicEventRequest and TopicEventResponse
cat spec/proto/runtime/v1/appcallback.proto | grep -A 5 "message TopicEvent"
Length of output: 708
| go func() { | ||
| time.Sleep(1 * time.Second) | ||
| ch := apiForTest.streamer.subscribers["___test||layotto"].publishResponses["1"] | ||
| ch <- &runtimev1pb.SubscribeTopicEventsRequestProcessed{ | ||
| Id: "1", | ||
| Status: &runtimev1pb.TopicEventResponse{ | ||
| Status: runtimev1pb.TopicEventResponse_SUCCESS, | ||
| }, | ||
| } | ||
| }() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace time.Sleep with proper synchronization
Using time.Sleep makes the test potentially flaky. Use a channel for proper synchronization.
+ done := make(chan struct{})
go func() {
- time.Sleep(1 * time.Second)
ch := apiForTest.streamer.subscribers["___test||layotto"].publishResponses["1"]
ch <- &runtimev1pb.SubscribeTopicEventsRequestProcessed{
Id: "1",
Status: &runtimev1pb.TopicEventResponse{
Status: runtimev1pb.TopicEventResponse_SUCCESS,
},
}
+ close(done)
}()
+ <-done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go func() { | |
| time.Sleep(1 * time.Second) | |
| ch := apiForTest.streamer.subscribers["___test||layotto"].publishResponses["1"] | |
| ch <- &runtimev1pb.SubscribeTopicEventsRequestProcessed{ | |
| Id: "1", | |
| Status: &runtimev1pb.TopicEventResponse{ | |
| Status: runtimev1pb.TopicEventResponse_SUCCESS, | |
| }, | |
| } | |
| }() | |
| done := make(chan struct{}) | |
| go func() { | |
| ch := apiForTest.streamer.subscribers["___test||layotto"].publishResponses["1"] | |
| ch <- &runtimev1pb.SubscribeTopicEventsRequestProcessed{ | |
| Id: "1", | |
| Status: &runtimev1pb.TopicEventResponse{ | |
| Status: runtimev1pb.TopicEventResponse_SUCCESS, | |
| }, | |
| } | |
| close(done) | |
| }() | |
| <-done |
| go func() { | ||
| defer wg.Done() | ||
| conn.notifyPublishResponse(stream.Context(), eventResp) | ||
| }() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid spawning a goroutine for each received message
In the Subscribe method, a new goroutine is spawned for every message received from the client to call conn.notifyPublishResponse. This can lead to resource exhaustion if messages are received at a high rate. Consider processing the messages sequentially within the loop, as notifyPublishResponse appears to be a lightweight operation.
Apply this diff to process messages sequentially:
for {
resp, err := stream.Recv()
// Error handling...
eventResp := resp.GetEventProcessed()
if eventResp == nil {
return errors.New("duplicate initial request received")
}
- wg.Add(1)
- go func() {
- defer wg.Done()
- conn.notifyPublishResponse(stream.Context(), eventResp)
- }()
+ conn.notifyPublishResponse(stream.Context(), eventResp)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go func() { | |
| defer wg.Done() | |
| conn.notifyPublishResponse(stream.Context(), eventResp) | |
| }() | |
| } | |
| conn.notifyPublishResponse(stream.Context(), eventResp) | |
| } |
| var resp *runtimev1pb.SubscribeTopicEventsRequestProcessed | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case resp = <-ch: | ||
| } | ||
|
|
||
| // 5. Check result | ||
| return retryStrategy(err, resp.Status, cloudEvent) | ||
| } |
There was a problem hiding this comment.
Ensure resp is not nil before using it
After receiving resp from the channel, there's no check to ensure that resp is not nil. Accessing resp.Status without this check could lead to a nil pointer dereference.
Apply this diff to check if resp is nil:
var resp *runtimev1pb.SubscribeTopicEventsRequestProcessed
select {
case <-ctx.Done():
return ctx.Err()
case resp = <-ch:
}
+if resp == nil {
+ return errors.New("received nil response")
+}
// 5. Check result
return retryStrategy(err, resp.Status, cloudEvent)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var resp *runtimev1pb.SubscribeTopicEventsRequestProcessed | |
| select { | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| case resp = <-ch: | |
| } | |
| // 5. Check result | |
| return retryStrategy(err, resp.Status, cloudEvent) | |
| } | |
| var resp *runtimev1pb.SubscribeTopicEventsRequestProcessed | |
| select { | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| case resp = <-ch: | |
| } | |
| if resp == nil { | |
| return errors.New("received nil response") | |
| } | |
| // 5. Check result | |
| return retryStrategy(err, resp.Status, cloudEvent) | |
| } |
| envelope, cloudEvent, _ := a.envelopeFromSubscriptionMessage(ctx, msg) | ||
|
|
There was a problem hiding this comment.
Handle the error returned by envelopeFromSubscriptionMessage
The error returned by a.envelopeFromSubscriptionMessage(ctx, msg) is currently ignored. If an error occurs, it should be handled to prevent unexpected behavior.
Apply this diff to handle the error:
envelope, cloudEvent, err := a.envelopeFromSubscriptionMessage(ctx, msg)
+if err != nil {
+ return err
+}Committable suggestion skipped: line range outside the PR's diff.
| a.streamer.lock.RLock() | ||
| key := a.streamer.StreamerKey(pubsubName, msg.Topic) | ||
| conn, ok := a.streamer.subscribers[key] | ||
| a.streamer.lock.RUnlock() | ||
| if !ok { | ||
| return fmt.Errorf("no streamer subscribed to pubsub %q topic %q", pubsubName, msg.Topic) | ||
| } |
There was a problem hiding this comment.
Check for nil before accessing a.streamer
In publishMessageForStream, a.streamer is accessed without checking if it is nil. This could lead to a nil pointer dereference if a.streamer has not been initialized.
Apply this diff to ensure a.streamer is not nil:
func (a *api) publishMessageForStream(ctx context.Context, msg *pubsub.NewMessage, pubsubName string) error {
+ if a.streamer == nil {
+ return errors.New("streamer is not initialized")
+ }
a.streamer.lock.RLock()
key := a.streamer.StreamerKey(pubsubName, msg.Topic)
conn, ok := a.streamer.subscribers[key]
a.streamer.lock.RUnlock()
if !ok {
return fmt.Errorf("no streamer subscribed to pubsub %q topic %q", pubsubName, msg.Topic)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| a.streamer.lock.RLock() | |
| key := a.streamer.StreamerKey(pubsubName, msg.Topic) | |
| conn, ok := a.streamer.subscribers[key] | |
| a.streamer.lock.RUnlock() | |
| if !ok { | |
| return fmt.Errorf("no streamer subscribed to pubsub %q topic %q", pubsubName, msg.Topic) | |
| } | |
| if a.streamer == nil { | |
| return errors.New("streamer is not initialized") | |
| } | |
| a.streamer.lock.RLock() | |
| key := a.streamer.StreamerKey(pubsubName, msg.Topic) | |
| conn, ok := a.streamer.subscribers[key] | |
| a.streamer.lock.RUnlock() | |
| if !ok { | |
| return fmt.Errorf("no streamer subscribed to pubsub %q topic %q", pubsubName, msg.Topic) | |
| } |
CrazyHZM
force-pushed
the
feat/support_SubscribeTopicEvents
branch
from
b486e46
to
a919731
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
pkg/grpc/default_api/api_pubsub.go (1)
218-219: Use appropriate log levels for error messagesThe error logging at lines 218 and 243 uses
Debugf, which may not bring sufficient attention to issues during deserialization and base64 decoding failures. Consider usingWarnforErrorfto ensure these errors are properly logged.Also applies to: 243-244
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
pkg/mock/runtime/runtime.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
demo/pubsub/README.md(1 hunks)pkg/grpc/default_api/api.go(2 hunks)pkg/grpc/default_api/api_pubsub.go(2 hunks)pkg/grpc/default_api/api_subscribe.go(1 hunks)pkg/grpc/default_api/api_subscribe_test.go(1 hunks)spec/proto/runtime/v1/runtime.proto(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- demo/pubsub/README.md
🔇 Additional comments (10)
pkg/grpc/default_api/api_subscribe_test.go (4)
131-132: Add assertions for published message content
The test should verify that the message was published with the correct content and metadata.
35-68: 🛠️ Refactor suggestion
Expand test coverage with additional scenarios
While the duplicate subscription test is well-implemented, consider adding more test cases using table-driven tests to cover:
- Successful subscription
- Invalid pubsub name
- Network errors (stream.Send fails)
- Subscription with invalid metadata
- Context cancellation scenarios
120-129: 🛠️ Refactor suggestion
Replace time.Sleep with proper synchronization
Using time.Sleep makes the test potentially flaky. Use a channel for proper synchronization.
70-134: 🛠️ Refactor suggestion
Add error test cases using table-driven tests
The test only covers the success case. Consider adding test cases for:
- Publishing to non-existent subscriber
- Publishing with invalid message format
- Context timeout scenarios
- Network errors during publishing
pkg/grpc/default_api/api.go (1)
153-155: Review concurrent operations architecture
The current implementation suggests incomplete concurrent operations support:
- The commented out
closeChinitialization indicates unfinished shutdown mechanism - Missing proper initialization of concurrent operation fields could affect the SubscribeTopicEvents API functionality
Consider implementing a proper shutdown mechanism:
- Initialize all concurrent operation fields in constructor
- Add a Shutdown method to the API interface
- Document the concurrent operation lifecycle
Let's verify the usage of these concurrent fields:
spec/proto/runtime/v1/runtime.proto (5)
73-75: Addition of 'SubscribeTopicEvents' RPC method appears correct.
The new RPC method SubscribeTopicEvents is appropriately defined with streaming request and response types. The addition aligns with the existing service definitions.
793-796: Ensure compatibility of 'optional' keyword in 'dead_letter_topic'.
The field optional string dead_letter_topic = 4; uses the optional keyword, which requires Protobuf compiler version 3.12 or later. Please ensure that the build environment uses a compatible compiler version to avoid potential compilation issues.
7-7: Ensure 'appcallback.proto' is correctly imported and exists in the codebase.
The import statement on line 7 references spec/proto/runtime/v1/appcallback.proto. Please confirm that this file exists in the codebase and is correctly located to prevent compilation errors.
Run the following script to verify the existence of appcallback.proto:
✅ Verification successful
Import statement is valid and file exists in the expected location
The file spec/proto/runtime/v1/appcallback.proto exists in the exact location referenced by the import statement, confirming that the import is correct and valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'appcallback.proto' exists in the codebase.
# Search for 'appcallback.proto' in the repository
fd 'appcallback.proto'
Length of output: 62
801-806: Verify that 'TopicEventResponse' is defined and properly imported.
The type TopicEventResponse used in SubscribeTopicEventsRequestProcessed may not be defined in this file. Ensure that it is defined in appcallback.proto and correctly imported to prevent compilation errors.
Run the following script to check if TopicEventResponse is defined:
✅ Verification successful
TopicEventResponse is properly defined and imported
The type TopicEventResponse is correctly defined in spec/proto/runtime/v1/appcallback.proto and is properly imported in runtime.proto via the import statement import "spec/proto/runtime/v1/appcallback.proto";. The message definition includes an enum TopicEventResponseStatus for handling message responses.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if 'TopicEventResponse' is defined in the codebase.
# Search for the definition of 'TopicEventResponse' in .proto files
ast-grep --pattern 'message TopicEventResponse { $$$ }'
Length of output: 57
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to search for TopicEventResponse
# First, let's search for the type definition
rg "message TopicEventResponse" -A 5
# Also search for any imports related to TopicEventResponse
rg "TopicEventResponse" --type proto
Length of output: 723
Script:
#!/bin/bash
# Let's verify if there's an import statement for appcallback.proto in runtime.proto
rg "import.*appcallback.proto" spec/proto/runtime/v1/runtime.proto
Length of output: 118
816-816: Verify that 'TopicEventRequest' is defined and properly imported.
The type TopicEventRequest used in SubscribeTopicEventsResponse may not be defined in this file. Ensure that it is defined in appcallback.proto and correctly imported to prevent compilation errors.
Run the following script to check if TopicEventRequest is defined:
✅ Verification successful
TopicEventRequest is properly defined in appcallback.proto
The TopicEventRequest message type is defined in spec/proto/runtime/v1/appcallback.proto. Let's verify if it's properly imported in the runtime.proto file where it's being used.
TopicEventRequest is properly defined and imported
The type is correctly defined in spec/proto/runtime/v1/appcallback.proto and properly imported in runtime.proto via the import statement import "spec/proto/runtime/v1/appcallback.proto";. The usage in SubscribeTopicEventsResponse is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if 'TopicEventRequest' is defined in the codebase.
# Search for the definition of 'TopicEventRequest' in .proto files
ast-grep --pattern 'message TopicEventRequest { $$$ }'
Length of output: 56
Script:
#!/bin/bash
# Search for TopicEventRequest definition using ripgrep since ast-grep might not support proto files
rg "message TopicEventRequest" -A 5
# Also search for any import statements related to TopicEventRequest
rg "import.*TopicEventRequest"
# List all proto files to understand the proto file structure
fd "\.proto$"
Length of output: 1097
Script:
#!/bin/bash
# Check imports in runtime.proto
rg "import.*appcallback" spec/proto/runtime/v1/runtime.proto
# Get some context around where TopicEventRequest is used in runtime.proto
rg "TopicEventRequest" spec/proto/runtime/v1/runtime.proto -B 2 -A 2
Length of output: 331
| streamer *streamer | ||
| // json | ||
| json jsoniter.API | ||
| json jsoniter.API | ||
| closeCh chan struct{} | ||
| wg sync.WaitGroup |
There was a problem hiding this comment.
Initialize all concurrent operation fields properly
The new fields added for concurrent operations need proper initialization in the constructor to prevent runtime issues:
streameris used without initialization, risking nil pointer dereferencecloseChinitialization is commented out, preventing proper shutdown signalingwgneeds initialization before use
Apply these changes in the constructor:
return &api{
// existing fields...
json: jsoniter.ConfigFastest,
+ streamer: newStreamer(),
+ closeCh: make(chan struct{}),
+ wg: sync.WaitGroup{},
}Committable suggestion skipped: line range outside the PR's diff.
| func (s *streamer) StreamerKey(pubsub, topic string) string { | ||
| return "___" + pubsub + "||" + topic | ||
| } |
There was a problem hiding this comment.
Prevent potential key collisions in StreamerKey
The StreamerKey function concatenates pubsub and topic using fixed separators ("___" and "||"). If pubsub or topic contain these separator strings, it could lead to key collisions or incorrect behavior.
Consider encoding pubsub and topic to avoid collisions, or use a different method for generating unique keys, such as hashing or a structured composite key.
Apply this diff to modify the StreamerKey function:
-func (s *streamer) StreamerKey(pubsub, topic string) string {
- return "___" + pubsub + "||" + topic
+func (s *streamer) StreamerKey(pubsub, topic string) string {
+ return fmt.Sprintf("pubsub:%s;topic:%s", pubsub, topic)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *streamer) StreamerKey(pubsub, topic string) string { | |
| return "___" + pubsub + "||" + topic | |
| } | |
| func (s *streamer) StreamerKey(pubsub, topic string) string { | |
| return fmt.Sprintf("pubsub:%s;topic:%s", pubsub, topic) | |
| } |
CrazyHZM
force-pushed
the
feat/support_SubscribeTopicEvents
branch
from
a919731
to
1a503bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
pkg/grpc/default_api/api_pubsub.go (1)
155-155: Implement tracing inpublishMessageGRPC.There's a
// TODO tracingcomment indicating that tracing needs to be implemented. Adding tracing will enhance observability and debugging capabilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
pkg/mock/runtime/runtime.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
demo/pubsub/README.md(1 hunks)pkg/grpc/default_api/api.go(2 hunks)pkg/grpc/default_api/api_pubsub.go(2 hunks)pkg/grpc/default_api/api_subscribe.go(1 hunks)pkg/grpc/default_api/api_subscribe_test.go(1 hunks)spec/proto/runtime/v1/runtime.proto(3 hunks)
🔇 Additional comments (8)
pkg/grpc/default_api/api.go (1)
89-93: Critical: Initialize concurrent operation fields properly
The newly added fields for concurrent operations require proper initialization in the constructor to prevent runtime issues:
streamer: Currently uninitialized, risking nil pointer dereference in subscription operationscloseCh: Channel initialization is commented out, which could prevent proper shutdown signalingwg: WaitGroup should be initialized before use
This issue was previously identified and remains unresolved. Please refer to the previous review comments for the suggested fixes.
pkg/grpc/default_api/api_pubsub.go (4)
234-241: Add validation and safe type assertions for cloud event fields.
The current implementation performs unsafe type assertions on the cloud event fields, which could lead to panics if the fields are missing or of unexpected types. Additionally, required cloud event fields are not being validated.
The issues highlighted are similar to previous review comments and are still applicable. Consider applying safe type assertions and validating required fields to ensure robustness.
245-249: Fix error variable in base64 decode error handling.
When handling the base64 decode error, the function returns err instead of decodeErr, which may lead to incorrect error propagation.
This issue was previously identified and remains unaddressed. Adjust the return statement to use decodeErr instead of err.
256-259: Handle JSON marshal errors and add safe type assertions.
The function ignores errors returned by a.json.Marshal(data) and performs unsafe type assertions on data.(string), which could cause panics.
As previously noted, it's important to handle potential errors from JSON marshaling and to perform safe type assertions to prevent runtime exceptions.
217-262: Validate cloud event fields to prevent runtime errors.
The function does not validate whether required cloud event fields are present, which could result in runtime errors if any fields are missing.
The prior review comment regarding the addition of validation for required fields is still valid. Implementing this validation will enhance the robustness of the function.
spec/proto/runtime/v1/runtime.proto (3)
73-76: Addition of 'SubscribeTopicEvents' RPC Method
The new SubscribeTopicEvents RPC method is appropriately defined and enhances the Pub/Sub capabilities by allowing clients to subscribe to topics via streaming. The method signature and accompanying comments are clear.
7-7: Verify the existence of 'appcallback.proto' at the specified path
Ensure that the file spec/proto/runtime/v1/appcallback.proto exists to prevent import errors during compilation.
Run the following script to confirm the existence of appcallback.proto:
✅ Verification successful
Import path verification successful
The file spec/proto/runtime/v1/appcallback.proto exists at the specified location, confirming that the import statement is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'appcallback.proto' exists at 'spec/proto/runtime/v1/'
# Test: Check if the file exists. Expected result: The file should be found.
if [ -f "spec/proto/runtime/v1/appcallback.proto" ]; then
echo "appcallback.proto exists at spec/proto/runtime/v1/"
else
echo "Error: appcallback.proto does not exist at spec/proto/runtime/v1/"
fi
Length of output: 163
764-823: Verify the definitions of 'TopicEventResponse' and 'TopicEventRequest'
The messages SubscribeTopicEventsRequestProcessed and SubscribeTopicEventsResponse reference TopicEventResponse and TopicEventRequest, respectively. Ensure that these types are properly defined and imported to prevent compilation errors.
Run the following script to confirm the definitions:
✅ Verification successful
Both TopicEventResponse and TopicEventRequest are properly defined in the codebase
The verification confirms that both message types are defined in spec/proto/runtime/v1/appcallback.proto:
TopicEventResponseis defined with its status enumTopicEventRequestis defined with its required fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'TopicEventResponse' and 'TopicEventRequest' are defined in the codebase.
# Test: Search for the message definitions. Expected result: Definitions are found.
# Searching for 'TopicEventResponse'
rg -A 5 "message TopicEventResponse"
# Searching for 'TopicEventRequest'
rg -A 5 "message TopicEventRequest"
Length of output: 1183
| @@ -2,7 +2,8 @@ You can run server/client demo with different component names. | |||
| It is worth noting that both server and client demo should set the same store name by param `-s`. | |||
| For example: | |||
| ```shell | |||
| cd ${project_path}/demo/pubsub/server/ | |||
| #################### Run pubsub demo with appcallback #################### | |||
| cd ${project_path}/demo/pubsub/appcallback/ | |||
There was a problem hiding this comment.
Address CI concerns with markdown shell commands
Based on the previous review comments, there are concerns about CI testing these markdown files. According to the documentation, adding new shell scripts directly might break CI.
Consider:
- Waiting for related PRs to be merged before adding this demo, as suggested in the previous discussion
- Ensuring the CI configuration is updated to handle these new paths
- Adding necessary test cases for the new demo path
| @@ -2,7 +2,8 @@ | |||
| It is worth noting that both server and client demo should set the same store name by param `-s`. | |||
| For example: | |||
| ```shell | |||
| cd ${project_path}/demo/pubsub/server/ | |||
| #################### Run pubsub demo with appcallback #################### | |||
There was a problem hiding this comment.
💡 Codebase verification
Directory path in README needs correction
The README.md contains an incorrect path. The directory structure shows server/appcallback/ but the README instructs to cd into demo/pubsub/appcallback/. This should be updated to demo/pubsub/server/appcallback/ to match the actual directory structure.
- Line 5: The section header is correct as it reflects the appcallback demo
- Line 6: Path should be corrected from
${project_path}/demo/pubsub/appcallback/to${project_path}/demo/pubsub/server/appcallback/
🔗 Analysis chain
Verify the section header matches the directory structure
Let's verify that the "appcallback" section header accurately reflects the directory structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the appcallback directory and its structure
# Expected: Directory should exist and contain the mentioned subscriber component
# Check if the appcallback directory exists
fd -t d "appcallback$" demo/pubsub
# List contents to verify the subscriber component
fd . demo/pubsub/appcallback -t f
Length of output: 215
Script:
#!/bin/bash
# Let's verify the actual structure and content of the pubsub demo directory
# to understand the correct organization
# Check the structure of demo/pubsub directory
fd . demo/pubsub -t d
# Look at the README.md content to understand the context better
cat demo/pubsub/README.md
Length of output: 754
| func TestSubscribeTopicEvents(t *testing.T) { | ||
| t.Run("SubscribeTopicEvents duplicate initial request", func(t *testing.T) { | ||
| pubsubName := "test" | ||
| topic := "layotto" | ||
| ctrl := gomock.NewController(t) | ||
| stream := runtime.NewMockRuntime_SubscribeTopicEventsServer(ctrl) | ||
| stream.EXPECT().Send(gomock.Any()).Return(nil).Times(1) | ||
| stream.EXPECT().Context().Return(context.Background()).AnyTimes() | ||
| stream.EXPECT().Recv().Return(&runtimev1pb.SubscribeTopicEventsRequest{ | ||
| SubscribeTopicEventsRequestType: &runtimev1pb.SubscribeTopicEventsRequest_InitialRequest{ | ||
| InitialRequest: &runtimev1pb.SubscribeTopicEventsRequestInitial{ | ||
| PubsubName: pubsubName, Topic: topic, | ||
| Metadata: make(map[string]string), | ||
| }, | ||
| }}, nil).AnyTimes() | ||
|
|
||
| a := NewAPI("", nil, nil, nil, make(map[string]pubsub.PubSub), nil, nil, nil, nil, nil, nil) | ||
|
|
||
| var apiForTest = a.(*api) | ||
|
|
||
| m := mock.NewMockPubSub(ctrl) | ||
| m.EXPECT().Subscribe(gomock.Any(), gomock.Any()).Return(nil).Times(1) | ||
| apiForTest.pubSubs["test"] = m | ||
|
|
||
| apiForTest.streamer = &streamer{ | ||
| subscribers: make(map[string]*conn), | ||
| } | ||
|
|
||
| apiForTest.json = jsoniter.ConfigFastest | ||
|
|
||
| err := apiForTest.SubscribeTopicEvents(stream) | ||
| assert.Error(t, err, "Expected error") | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand test coverage with table-driven tests
While the duplicate subscription test is well-implemented, consider adding more test cases using table-driven tests to ensure comprehensive coverage:
- Successful subscription
- Invalid pubsub name
- Invalid topic name
- Network errors (stream.Send fails)
- Invalid metadata
func TestSubscribeTopicEvents(t *testing.T) {
+ tests := []struct {
+ name string
+ pubsubName string
+ topic string
+ metadata map[string]string
+ setupMock func(*gomock.Controller) (*api, *runtime.MockRuntime_SubscribeTopicEventsServer)
+ wantErr bool
+ errMsg string
+ }{
+ {
+ name: "successful subscription",
+ pubsubName: "test",
+ topic: "topic1",
+ metadata: map[string]string{"key": "value"},
+ setupMock: func(ctrl *gomock.Controller) (*api, *runtime.MockRuntime_SubscribeTopicEventsServer) {
+ // Setup for success case
+ return setupMockAPI(ctrl, true)
+ },
+ wantErr: false,
+ },
+ {
+ name: "duplicate subscription",
+ pubsubName: "test",
+ topic: "layotto",
+ metadata: make(map[string]string),
+ setupMock: func(ctrl *gomock.Controller) (*api, *runtime.MockRuntime_SubscribeTopicEventsServer) {
+ // Existing test setup
+ return setupMockAPI(ctrl, false)
+ },
+ wantErr: true,
+ errMsg: "duplicate subscription",
+ },
+ // Add more test cases
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ defer ctrl.Finish()
+
+ api, stream := tt.setupMock(ctrl)
+ err := api.SubscribeTopicEvents(stream)
+
+ if tt.wantErr {
+ assert.Error(t, err)
+ if tt.errMsg != "" {
+ assert.Contains(t, err.Error(), tt.errMsg)
+ }
+ } else {
+ assert.NoError(t, err)
+ }
+ })
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| func TestPublishMessageForStream(t *testing.T) { | ||
| t.Run("publish success", func(t *testing.T) { | ||
| pubsubName := "test" | ||
| topic := "layotto" | ||
| ctrl := gomock.NewController(t) | ||
| stream := runtime.NewMockRuntime_SubscribeTopicEventsServer(ctrl) | ||
| stream.EXPECT().Send(gomock.Any()).Return(nil).Times(1) | ||
| stream.EXPECT().Recv().Return(&runtimev1pb.SubscribeTopicEventsRequest{ | ||
| SubscribeTopicEventsRequestType: &runtimev1pb.SubscribeTopicEventsRequest_InitialRequest{ | ||
| InitialRequest: &runtimev1pb.SubscribeTopicEventsRequestInitial{ | ||
| PubsubName: pubsubName, Topic: topic, | ||
| Metadata: make(map[string]string), | ||
| }, | ||
| }}, nil).Times(1) | ||
|
|
||
| cloudEvent := map[string]interface{}{ | ||
| pubsub.IDField: "1", | ||
| pubsub.SourceField: "source", | ||
| pubsub.DataContentTypeField: "content-type", | ||
| pubsub.TypeField: "type", | ||
| pubsub.SpecVersionField: "v1.0.0", | ||
| pubsub.DataBase64Field: "bGF5b3R0bw==", | ||
| } | ||
|
|
||
| data, err := json.Marshal(cloudEvent) | ||
| assert.Nil(t, err) | ||
|
|
||
| msg := &pubsub.NewMessage{ | ||
| Data: data, | ||
| Topic: topic, | ||
| Metadata: make(map[string]string), | ||
| } | ||
| a := NewAPI("", nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) | ||
|
|
||
| var apiForTest = a.(*api) | ||
|
|
||
| apiForTest.streamer = &streamer{ | ||
| subscribers: make(map[string]*conn), | ||
| } | ||
|
|
||
| apiForTest.streamer.subscribers["___test||layotto"] = &conn{ | ||
| stream: stream, | ||
| publishResponses: make(map[string]chan *runtimev1pb.SubscribeTopicEventsRequestProcessed), | ||
| } | ||
|
|
||
| err = apiForTest.SubscribeTopicEvents(stream) | ||
| assert.Nil(t, err) | ||
|
|
||
| apiForTest.json = jsoniter.ConfigFastest | ||
|
|
||
| go func() { | ||
| time.Sleep(1 * time.Second) | ||
| ch := apiForTest.streamer.subscribers["___test||layotto"].publishResponses["1"] | ||
| ch <- &runtimev1pb.SubscribeTopicEventsRequestProcessed{ | ||
| Id: "1", | ||
| Status: &runtimev1pb.TopicEventResponse{ | ||
| Status: runtimev1pb.TopicEventResponse_SUCCESS, | ||
| }, | ||
| } | ||
| }() | ||
|
|
||
| err = apiForTest.publishMessageForStream(context.Background(), msg, pubsubName) | ||
| assert.Nil(t, err) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test reliability and coverage
The test has several areas for improvement:
- The use of
time.Sleepmakes the test flaky (as noted in previous reviews) - Missing message content validation (as noted in previous reviews)
- Limited error scenario coverage
Consider adding these error test cases using table-driven tests:
- Non-existent subscriber
- Invalid message format
- Channel closed
- Context cancellation
func TestPublishMessageForStream(t *testing.T) {
+ tests := []struct {
+ name string
+ msg *pubsub.NewMessage
+ setupMock func(*gomock.Controller) (*api, context.Context, context.CancelFunc)
+ wantErr bool
+ errMsg string
+ }{
+ {
+ name: "publish success",
+ msg: createTestMessage("1", "layotto", "test-data"),
+ setupMock: func(ctrl *gomock.Controller) (*api, context.Context, context.CancelFunc) {
+ // Existing success case setup
+ return setupTestAPI(ctrl)
+ },
+ wantErr: false,
+ },
+ {
+ name: "context cancelled",
+ msg: createTestMessage("2", "layotto", "test-data"),
+ setupMock: func(ctrl *gomock.Controller) (*api, context.Context, context.CancelFunc) {
+ ctx, cancel := context.WithCancel(context.Background())
+ api, _, _ := setupTestAPI(ctrl)
+ cancel() // Cancel context immediately
+ return api, ctx, nil
+ },
+ wantErr: true,
+ errMsg: "context canceled",
+ },
+ // Add more test cases
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ defer ctrl.Finish()
+
+ api, ctx, cancel := tt.setupMock(ctrl)
+ if cancel != nil {
+ defer cancel()
+ }
+
+ err := api.publishMessageForStream(ctx, tt.msg, "test")
+
+ if tt.wantErr {
+ assert.Error(t, err)
+ if tt.errMsg != "" {
+ assert.Contains(t, err.Error(), tt.errMsg)
+ }
+ } else {
+ assert.NoError(t, err)
+ }
+ })
+ }
}
+ func createTestMessage(id, topic, data string) *pubsub.NewMessage {
+ cloudEvent := map[string]interface{}{
+ pubsub.IDField: id,
+ pubsub.SourceField: "source",
+ pubsub.DataContentTypeField: "content-type",
+ pubsub.TypeField: "type",
+ pubsub.SpecVersionField: "v1.0.0",
+ pubsub.DataBase64Field: data,
+ }
+
+ eventData, _ := json.Marshal(cloudEvent)
+ return &pubsub.NewMessage{
+ Data: eventData,
+ Topic: topic,
+ Metadata: make(map[string]string),
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
| //closeCh: make(chan struct{}), | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Critical: Constructor must initialize streamer field
The streamer field requires initialization in the constructor as it's actively used in subscription handling:
- The field is used in
Subscribeoperations without nil checks - Tests explicitly initialize it with
subscribersmap - The struct definition shows it contains critical synchronization primitives
Required initialization:
streamer: &streamer{
subscribers: make(map[string]*conn),
},🔗 Analysis chain
Critical: Constructor missing required field initializations
The constructor only initializes the json field but leaves other critical fields uninitialized:
- The commented out
closeChinitialization suggests it's needed but was deliberately disabled - Missing initialization for
streamerandwgfields
Let's verify the usage of these fields in the codebase:
Let me gather more context about the struct definition and initialization patterns.
Let me gather more information about the streamer initialization and struct definition.
Let me check one more thing about the streamer field and its initialization in the constructor.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify field usage patterns that could lead to runtime issues
# Check for streamer usage without nil checks
echo "Checking streamer usage patterns..."
ast-grep --pattern 'a.streamer.$_'
# Check for closeCh usage
echo "Checking closeCh usage patterns..."
rg 'closeCh.*<-'
# Check for WaitGroup usage
echo "Checking WaitGroup usage patterns..."
rg 'wg\.(Add|Done|Wait)'
Length of output: 5831
Script:
#!/bin/bash
# Check the struct definition and field types
echo "Checking struct definition..."
rg -A 10 "type API struct"
# Check initialization patterns
echo "Checking initialization patterns..."
rg "New.*API" -A 5
# Check if streamer is initialized elsewhere
echo "Checking streamer initialization..."
rg "streamer.*=.*new\("
Length of output: 58900
Script:
#!/bin/bash
# Check the struct definition and streamer field usage
echo "Checking API struct definition..."
rg -B 2 -A 10 "type api struct"
# Check streamer initialization in the codebase
echo "Checking streamer initialization patterns..."
rg "streamer.*=.*{" -A 3
# Check if streamer is initialized in tests
echo "Checking test initialization patterns..."
rg "apiForTest.streamer.*=.*" -A 3
Length of output: 2264
Script:
#!/bin/bash
# Check the streamer field in the struct and its initialization
echo "Checking streamer field and initialization..."
rg -B 5 -A 10 "type streamer struct"
# Check NewAPI implementation
echo "Checking NewAPI implementation..."
rg -B 5 -A 15 "func NewAPI\("
Length of output: 7913
| var wg sync.WaitGroup | ||
| defer wg.Wait() | ||
| for { |
There was a problem hiding this comment.
Avoid potential deadlock caused by deferred wg.Wait()
Deferring wg.Wait() at the beginning of the function can lead to a deadlock if no goroutines are spawned or if an early return occurs before any goroutines are added to the wait group. This can cause the function to hang indefinitely.
Consider moving wg.Wait() to just before the function returns, after the loop:
var wg sync.WaitGroup
- defer wg.Wait()
for {
resp, err := stream.Recv()
// ... existing code ...
}
+ wg.Wait()Committable suggestion skipped: line range outside the PR's diff.
Signed-off-by: “huazhongming” <[email protected]>
CrazyHZM
force-pushed
the
feat/support_SubscribeTopicEvents
branch
from
1a503bb
to
a258f97
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
pkg/grpc/default_api/api_subscribe_test.go (1)
35-68: Enhance test coverage and assertionsThe test could be improved in several ways:
- The test name "SubscribeTopicEvents duplicate initial request" could be more descriptive
- The error assertion should verify the specific error message
- Consider adding more test cases using table-driven tests as suggested in past reviews
Here's a suggested improvement for the current test case:
-func TestSubscribeTopicEvents(t *testing.T) { - t.Run("SubscribeTopicEvents duplicate initial request", func(t *testing.T) { +func TestSubscribeTopicEvents(t *testing.T) { + t.Run("should return error when attempting duplicate subscription to same topic", func(t *testing.T) { // ... existing setup ... err := apiForTest.SubscribeTopicEvents(stream) - assert.Error(t, err, "Expected error") + assert.Error(t, err) + assert.Contains(t, err.Error(), "duplicate subscription")pkg/grpc/default_api/api_subscribe.go (1)
143-144: Update log tag to reflect the correct function nameThe log tag in the warning message uses
[runtime][beginPubSub], which does not match the current functionstreamSubscribe. For consistency and clarity, consider updating the log tag.Apply this diff to correct the log tag:
- log.DefaultLogger.Warnf("[runtime][beginPubSub]failed to subscribe to topic %s: %s", initialRequest.Topic, err) + log.DefaultLogger.Warnf("[runtime][streamSubscribe]failed to subscribe to topic %s: %s", initialRequest.Topic, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
pkg/mock/runtime/runtime.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime.pb.gois excluded by!**/*.pb.gospec/proto/runtime/v1/runtime_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
demo/pubsub/README.md(1 hunks)pkg/grpc/default_api/api.go(2 hunks)pkg/grpc/default_api/api_pubsub.go(2 hunks)pkg/grpc/default_api/api_subscribe.go(1 hunks)pkg/grpc/default_api/api_subscribe_test.go(1 hunks)spec/proto/runtime/v1/runtime.proto(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- demo/pubsub/README.md
🧰 Additional context used
🪛 GitHub Check: Go CI Linter (.)
pkg/grpc/default_api/api_subscribe_test.go
[failure] 115-115:
ineffectual assignment to err (ineffassign)
🔇 Additional comments (16)
pkg/grpc/default_api/api_subscribe_test.go (2)
118-127:
Replace time.Sleep with proper synchronization
Using time.Sleep makes the test flaky. Use a channel for proper synchronization as suggested in past reviews.
+ done := make(chan struct{})
go func() {
- time.Sleep(1 * time.Second)
ch := apiForTest.streamer.subscribers["___test||layotto"].publishResponses["1"]
ch <- &runtimev1pb.SubscribeTopicEventsRequestProcessed{
Id: "1",
Status: &runtimev1pb.TopicEventResponse{
Status: runtimev1pb.TopicEventResponse_SUCCESS,
},
}
+ close(done)
}()
+ <-doneLikely invalid or redundant comment.
129-130: 🛠️ Refactor suggestion
Add assertions for published message content
The test should verify that the message was published with the correct content and metadata.
err = apiForTest.publishMessageForStream(context.Background(), msg, pubsubName)
assert.Nil(t, err)
+ // Verify the published message
+ assert.Equal(t, topic, msg.Topic)
+ var publishedEvent map[string]interface{}
+ err = json.Unmarshal(msg.Data, &publishedEvent)
+ assert.Nil(t, err)
+ assert.Equal(t, "1", publishedEvent[pubsub.IDField])
+ assert.Equal(t, "source", publishedEvent[pubsub.SourceField])
+ assert.Equal(t, "bGF5b3R0bw==", publishedEvent[pubsub.DataBase64Field])Likely invalid or redundant comment.
pkg/grpc/default_api/api.go (2)
89-93:
Critical: Initialize all concurrent operation fields properly
The new fields added for concurrent operations require proper initialization in the constructor to prevent runtime issues:
streameris critical for subscription handling and needs initializationwgis used for goroutine synchronizationcloseChis needed for graceful shutdown signaling
Apply these changes in the constructor:
return &api{
// existing fields...
json: jsoniter.ConfigFastest,
+ streamer: newStreamer(),
+ closeCh: make(chan struct{}),
+ wg: sync.WaitGroup{},
}Likely invalid or redundant comment.
153-155:
Uncomment closeCh initialization and add missing field initializations
The commented out closeCh initialization and missing initializations for other fields could lead to runtime issues:
- Without
closeCh, proper shutdown signaling won't work - Missing
streamerinitialization could cause nil pointer dereferences - Missing
wginitialization could cause synchronization issues
Let's verify the usage of these fields in the codebase:
pkg/grpc/default_api/api_subscribe.go (3)
49-69: LGTM: The SubscribeTopicEvents function is implemented correctly
The function initializes error channels and handles subscription logic appropriately.
71-148: LGTM: The streamSubscribe function manages subscriptions effectively
The function processes the initial request, validates input parameters, and sets up subscriptions successfully.
204-235: LGTM: The publishMessageForStream function processes messages correctly
The function retrieves the subscriber connection and sends messages appropriately, handling errors and responses.
pkg/grpc/default_api/api_pubsub.go (3)
156-162: Good job adding error handling and nil checks
The added error handling for envelopeFromSubscriptionMessage and the nil check for envelope ensure that potential errors and expired messages are properly handled, preventing panics and unintended behavior.
245-249:
Fix error variable in base64 decode error handling
The error handling for base64 decoding returns the wrong error variable. It should return decodeErr instead of err to accurately reflect the decoding error.
Apply this diff to correct the error handling:
decoded, decodeErr := base64.StdEncoding.DecodeString(data.(string))
if decodeErr != nil {
log.DefaultLogger.Debugf("unable to base64 decode cloudEvent field data_base64: %s", decodeErr)
- return nil, cloudEvent, err
+ return nil, cloudEvent, decodeErr
}Likely invalid or redundant comment.
255-259:
Add safe type assertions and handle JSON marshal errors
Unsafe type assertions and ignoring JSON marshal errors can lead to panics or data inconsistencies. It's crucial to verify types and handle errors appropriately.
Consider applying this diff to enhance type safety and error handling:
if contenttype.IsStringContentType(envelope.DataContentType) {
- envelope.Data = []byte(data.(string))
+ dataStr, ok := data.(string)
+ if !ok {
+ return nil, cloudEvent, fmt.Errorf("invalid type for data field: expected string")
+ }
+ envelope.Data = []byte(dataStr)
} else if contenttype.IsJSONContentType(envelope.DataContentType) {
- envelope.Data, _ = a.json.Marshal(data)
+ var err error
+ envelope.Data, err = a.json.Marshal(data)
+ if err != nil {
+ return nil, cloudEvent, fmt.Errorf("failed to marshal data field: %v", err)
+ }
}Likely invalid or redundant comment.
spec/proto/runtime/v1/runtime.proto (6)
73-76: Addition of 'SubscribeTopicEvents' RPC Method
The SubscribeTopicEvents method is correctly defined as a bidirectional streaming RPC in the Runtime service. This enhances the pub/sub functionality effectively.
764-776: Validate 'SubscribeTopicEventsRequest' Message Structure
The SubscribeTopicEventsRequest message uses a oneof field to differentiate between initial_request and event_processed, which is appropriate for streaming requests. The structure and field naming are consistent with Protobuf best practices.
793-796: Ensure Protobuf Compiler Compatibility with 'optional' Keyword
The field dead_letter_topic is declared with the optional keyword:
optional string dead_letter_topic = 4;Please ensure that your development environment uses protoc compiler version 3.12 or higher, as the optional keyword is only supported from that version onwards.
7-7: Confirm 'appcallback.proto' Import and Type Definitions
The import statement import "spec/proto/runtime/v1/appcallback.proto"; introduces a dependency on appcallback.proto. Please verify that:
- The file
appcallback.protoexists at the specified path. - It defines the message types
TopicEventRequestandTopicEventResponseused in this file.
Run the following script to confirm the existence of appcallback.proto and the required message types:
✅ Verification successful
Import and Message Type Definitions Verified Successfully
The verification confirms:
appcallback.protoexists at the correct path:spec/proto/runtime/v1/appcallback.proto- Both required message types are defined in the file:
TopicEventRequestmessage type is presentTopicEventResponsemessage type is present with its status enum
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'appcallback.proto' exists and contains the necessary message types.
# Check for 'appcallback.proto' in the repository
fd "appcallback.proto"
# Verify that 'TopicEventRequest' is defined
rg -A 3 'message TopicEventRequest' $(fd "appcallback.proto")
# Verify that 'TopicEventResponse' is defined
rg -A 3 'message TopicEventResponse' $(fd "appcallback.proto")
Length of output: 746
801-806: Confirm Definition of 'TopicEventResponse'
In SubscribeTopicEventsRequestProcessed, the field status is of type TopicEventResponse. Ensure that this type is defined in appcallback.proto and properly imported.
Run the following script to verify the definition:
✅ Verification successful
TopicEventResponse is Properly Defined and Imported
The type TopicEventResponse is correctly defined in appcallback.proto and properly imported in runtime.proto. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'TopicEventResponse' is defined in 'appcallback.proto'
# Search for 'TopicEventResponse' definition
rg -A 3 'message TopicEventResponse' $(fd "appcallback.proto")
Length of output: 350
Script:
#!/bin/bash
# Check if TopicEventResponse is imported in runtime.proto
rg -A 3 'import.*appcallback.proto' spec/proto/runtime/v1/runtime.proto
Length of output: 239
816-817: Confirm Definition of 'TopicEventRequest'
In SubscribeTopicEventsResponse, the field event_message is of type TopicEventRequest. Please verify that this type is defined in appcallback.proto and correctly referenced.
Run the following script to verify the definition:
✅ Verification successful
Type TopicEventRequest is properly defined and imported
The verification confirms that:
TopicEventRequestis defined inspec/proto/runtime/v1/appcallback.protoappcallback.protois correctly imported inruntime.proto
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'TopicEventRequest' is defined in 'appcallback.proto'
# Search for 'TopicEventRequest' definition
rg -A 3 'message TopicEventRequest' $(fd "appcallback.proto")
Length of output: 332
Script:
#!/bin/bash
# Let's verify the import statements in runtime.proto to ensure TopicEventRequest is properly imported
rg -A 3 "^import.*appcallback.proto" spec/proto/runtime/v1/runtime.proto
Length of output: 240
| func TestPublishMessageForStream(t *testing.T) { | ||
| t.Run("publish success", func(t *testing.T) { | ||
| pubsubName := "test" | ||
| topic := "layotto" | ||
| ctrl := gomock.NewController(t) | ||
| stream := runtime.NewMockRuntime_SubscribeTopicEventsServer(ctrl) | ||
| stream.EXPECT().Send(gomock.Any()).Return(nil).Times(1) | ||
| stream.EXPECT().Recv().Return(&runtimev1pb.SubscribeTopicEventsRequest{ | ||
| SubscribeTopicEventsRequestType: &runtimev1pb.SubscribeTopicEventsRequest_InitialRequest{ | ||
| InitialRequest: &runtimev1pb.SubscribeTopicEventsRequestInitial{ | ||
| PubsubName: pubsubName, Topic: topic, | ||
| Metadata: make(map[string]string), | ||
| }, | ||
| }}, nil).Times(1) | ||
|
|
||
| cloudEvent := map[string]interface{}{ | ||
| pubsub.IDField: "1", | ||
| pubsub.SourceField: "source", | ||
| pubsub.DataContentTypeField: "content-type", | ||
| pubsub.TypeField: "type", | ||
| pubsub.SpecVersionField: "v1.0.0", | ||
| pubsub.DataBase64Field: "bGF5b3R0bw==", | ||
| } | ||
|
|
||
| data, err := json.Marshal(cloudEvent) | ||
| assert.Nil(t, err) | ||
|
|
||
| msg := &pubsub.NewMessage{ | ||
| Data: data, | ||
| Topic: topic, | ||
| Metadata: make(map[string]string), | ||
| } | ||
| a := NewAPI("", nil, nil, nil, make(map[string]pubsub.PubSub), nil, nil, nil, nil, nil, nil) | ||
|
|
||
| var apiForTest = a.(*api) | ||
|
|
||
| apiForTest.streamer = &streamer{ | ||
| subscribers: make(map[string]*conn), | ||
| } | ||
|
|
||
| apiForTest.streamer.subscribers["___test||layotto"] = &conn{ | ||
| stream: stream, | ||
| publishResponses: make(map[string]chan *runtimev1pb.SubscribeTopicEventsRequestProcessed), | ||
| } | ||
|
|
||
| err = apiForTest.SubscribeTopicEvents(stream) | ||
| apiForTest.json = jsoniter.ConfigFastest | ||
|
|
||
| go func() { | ||
| time.Sleep(1 * time.Second) | ||
| ch := apiForTest.streamer.subscribers["___test||layotto"].publishResponses["1"] | ||
| ch <- &runtimev1pb.SubscribeTopicEventsRequestProcessed{ | ||
| Id: "1", | ||
| Status: &runtimev1pb.TopicEventResponse{ | ||
| Status: runtimev1pb.TopicEventResponse_SUCCESS, | ||
| }, | ||
| } | ||
| }() | ||
|
|
||
| err = apiForTest.publishMessageForStream(context.Background(), msg, pubsubName) | ||
| assert.Nil(t, err) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error test cases
The test only covers the success scenario. Consider adding test cases for error scenarios using table-driven tests.
Key scenarios to test:
- Non-existent subscriber
- Invalid message format
- Channel closed
- Context cancellation
Would you like me to help generate the table-driven test implementation?
🧰 Tools
🪛 GitHub Check: Go CI Linter (.)
[failure] 115-115:
ineffectual assignment to err (ineffassign)
| publishResponses: make(map[string]chan *runtimev1pb.SubscribeTopicEventsRequestProcessed), | ||
| } | ||
|
|
||
| err = apiForTest.SubscribeTopicEvents(stream) |
There was a problem hiding this comment.
Fix ineffectual error assignment
The error returned from SubscribeTopicEvents is assigned but never checked.
-err = apiForTest.SubscribeTopicEvents(stream)
+err := apiForTest.SubscribeTopicEvents(stream)
+assert.NoError(t, err)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Go CI Linter (.)
[failure] 115-115:
ineffectual assignment to err (ineffassign)
| func (a *api) envelopeFromSubscriptionMessage(ctx context.Context, msg *pubsub.NewMessage) (*runtimev1pb.TopicEventRequest, map[string]interface{}, error) { | ||
| // 1. Unmarshal to cloudEvent model | ||
| var cloudEvent map[string]interface{} | ||
| err := a.json.Unmarshal(msg.Data, &cloudEvent) | ||
| if err != nil { | ||
| log.DefaultLogger.Debugf("[runtime]error deserializing cloud events proto: %s", err) | ||
| return nil, cloudEvent, err | ||
| } | ||
|
|
||
| // 2. Drop msg if the current cloud event has expired | ||
| if pubsub.HasExpired(cloudEvent) { | ||
| log.DefaultLogger.Warnf("[runtime]dropping expired pub/sub event %v as of %v", cloudEvent[pubsub.IDField].(string), cloudEvent[pubsub.ExpirationField].(string)) | ||
| return nil, cloudEvent, nil | ||
| } | ||
|
|
||
| // 3. Convert to proto domain struct | ||
| envelope := &runtimev1pb.TopicEventRequest{ | ||
| Id: cloudEvent[pubsub.IDField].(string), | ||
| Source: cloudEvent[pubsub.SourceField].(string), | ||
| DataContentType: cloudEvent[pubsub.DataContentTypeField].(string), | ||
| Type: cloudEvent[pubsub.TypeField].(string), | ||
| SpecVersion: cloudEvent[pubsub.SpecVersionField].(string), | ||
| Topic: msg.Topic, | ||
| PubsubName: msg.Metadata[Metadata_key_pubsubName], | ||
| } | ||
|
|
||
| // set data field | ||
| if data, ok := cloudEvent[pubsub.DataBase64Field]; ok && data != nil { | ||
| decoded, decodeErr := base64.StdEncoding.DecodeString(data.(string)) | ||
| if decodeErr != nil { | ||
| log.DefaultLogger.Debugf("unable to base64 decode cloudEvent field data_base64: %s", decodeErr) | ||
| return nil, cloudEvent, err | ||
| } | ||
|
|
||
| envelope.Data = decoded | ||
| } else if data, ok := cloudEvent[pubsub.DataField]; ok && data != nil { | ||
| envelope.Data = nil | ||
|
|
||
| if contenttype.IsStringContentType(envelope.DataContentType) { | ||
| envelope.Data = []byte(data.(string)) | ||
| } else if contenttype.IsJSONContentType(envelope.DataContentType) { | ||
| envelope.Data, _ = a.json.Marshal(data) | ||
| } | ||
| } | ||
| return envelope, cloudEvent, nil | ||
| } |
There was a problem hiding this comment.
Add validation for required cloud event fields and safe type assertions
The current implementation lacks validation for required cloud event fields and performs unsafe type assertions, which could lead to panics if the data is not as expected.
Please add validation for required fields and use safe type assertions to enhance robustness. Consider applying the following changes:
+ // Validate required fields
+ requiredFields := []string{pubsub.IDField, pubsub.SourceField, pubsub.DataContentTypeField, pubsub.TypeField, pubsub.SpecVersionField}
+ for _, field := range requiredFields {
+ if _, ok := cloudEvent[field]; !ok {
+ return nil, cloudEvent, fmt.Errorf("required field %s missing from cloud event", field)
+ }
+ }
+
+ // Safe type assertions
+ id, ok := cloudEvent[pubsub.IDField].(string)
+ if !ok {
+ return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.IDField)
+ }
+ source, ok := cloudEvent[pubsub.SourceField].(string)
+ if !ok {
+ return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.SourceField)
+ }
+ contentType, ok := cloudEvent[pubsub.DataContentTypeField].(string)
+ if !ok {
+ return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.DataContentTypeField)
+ }
+ eventType, ok := cloudEvent[pubsub.TypeField].(string)
+ if !ok {
+ return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.TypeField)
+ }
+ specVersion, ok := cloudEvent[pubsub.SpecVersionField].(string)
+ if !ok {
+ return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.SpecVersionField)
+ }
+
envelope := &runtimev1pb.TopicEventRequest{
- Id: cloudEvent[pubsub.IDField].(string),
- Source: cloudEvent[pubsub.SourceField].(string),
- DataContentType: cloudEvent[pubsub.DataContentTypeField].(string),
- Type: cloudEvent[pubsub.TypeField].(string),
- SpecVersion: cloudEvent[pubsub.SpecVersionField].(string),
+ Id: id,
+ Source: source,
+ DataContentType: contentType,
+ Type: eventType,
+ SpecVersion: specVersion,
Topic: msg.Topic,
PubsubName: msg.Metadata[Metadata_key_pubsubName],
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (a *api) envelopeFromSubscriptionMessage(ctx context.Context, msg *pubsub.NewMessage) (*runtimev1pb.TopicEventRequest, map[string]interface{}, error) { | |
| // 1. Unmarshal to cloudEvent model | |
| var cloudEvent map[string]interface{} | |
| err := a.json.Unmarshal(msg.Data, &cloudEvent) | |
| if err != nil { | |
| log.DefaultLogger.Debugf("[runtime]error deserializing cloud events proto: %s", err) | |
| return nil, cloudEvent, err | |
| } | |
| // 2. Drop msg if the current cloud event has expired | |
| if pubsub.HasExpired(cloudEvent) { | |
| log.DefaultLogger.Warnf("[runtime]dropping expired pub/sub event %v as of %v", cloudEvent[pubsub.IDField].(string), cloudEvent[pubsub.ExpirationField].(string)) | |
| return nil, cloudEvent, nil | |
| } | |
| // 3. Convert to proto domain struct | |
| envelope := &runtimev1pb.TopicEventRequest{ | |
| Id: cloudEvent[pubsub.IDField].(string), | |
| Source: cloudEvent[pubsub.SourceField].(string), | |
| DataContentType: cloudEvent[pubsub.DataContentTypeField].(string), | |
| Type: cloudEvent[pubsub.TypeField].(string), | |
| SpecVersion: cloudEvent[pubsub.SpecVersionField].(string), | |
| Topic: msg.Topic, | |
| PubsubName: msg.Metadata[Metadata_key_pubsubName], | |
| } | |
| // set data field | |
| if data, ok := cloudEvent[pubsub.DataBase64Field]; ok && data != nil { | |
| decoded, decodeErr := base64.StdEncoding.DecodeString(data.(string)) | |
| if decodeErr != nil { | |
| log.DefaultLogger.Debugf("unable to base64 decode cloudEvent field data_base64: %s", decodeErr) | |
| return nil, cloudEvent, err | |
| } | |
| envelope.Data = decoded | |
| } else if data, ok := cloudEvent[pubsub.DataField]; ok && data != nil { | |
| envelope.Data = nil | |
| if contenttype.IsStringContentType(envelope.DataContentType) { | |
| envelope.Data = []byte(data.(string)) | |
| } else if contenttype.IsJSONContentType(envelope.DataContentType) { | |
| envelope.Data, _ = a.json.Marshal(data) | |
| } | |
| } | |
| return envelope, cloudEvent, nil | |
| } | |
| func (a *api) envelopeFromSubscriptionMessage(ctx context.Context, msg *pubsub.NewMessage) (*runtimev1pb.TopicEventRequest, map[string]interface{}, error) { | |
| // 1. Unmarshal to cloudEvent model | |
| var cloudEvent map[string]interface{} | |
| err := a.json.Unmarshal(msg.Data, &cloudEvent) | |
| if err != nil { | |
| log.DefaultLogger.Debugf("[runtime]error deserializing cloud events proto: %s", err) | |
| return nil, cloudEvent, err | |
| } | |
| // Validate required fields | |
| requiredFields := []string{pubsub.IDField, pubsub.SourceField, pubsub.DataContentTypeField, pubsub.TypeField, pubsub.SpecVersionField} | |
| for _, field := range requiredFields { | |
| if _, ok := cloudEvent[field]; !ok { | |
| return nil, cloudEvent, fmt.Errorf("required field %s missing from cloud event", field) | |
| } | |
| } | |
| // Safe type assertions | |
| id, ok := cloudEvent[pubsub.IDField].(string) | |
| if !ok { | |
| return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.IDField) | |
| } | |
| source, ok := cloudEvent[pubsub.SourceField].(string) | |
| if !ok { | |
| return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.SourceField) | |
| } | |
| contentType, ok := cloudEvent[pubsub.DataContentTypeField].(string) | |
| if !ok { | |
| return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.DataContentTypeField) | |
| } | |
| eventType, ok := cloudEvent[pubsub.TypeField].(string) | |
| if !ok { | |
| return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.TypeField) | |
| } | |
| specVersion, ok := cloudEvent[pubsub.SpecVersionField].(string) | |
| if !ok { | |
| return nil, cloudEvent, fmt.Errorf("invalid type for field %s: expected string", pubsub.SpecVersionField) | |
| } | |
| // 2. Drop msg if the current cloud event has expired | |
| if pubsub.HasExpired(cloudEvent) { | |
| log.DefaultLogger.Warnf("[runtime]dropping expired pub/sub event %v as of %v", cloudEvent[pubsub.IDField].(string), cloudEvent[pubsub.ExpirationField].(string)) | |
| return nil, cloudEvent, nil | |
| } | |
| // 3. Convert to proto domain struct | |
| envelope := &runtimev1pb.TopicEventRequest{ | |
| Id: id, | |
| Source: source, | |
| DataContentType: contentType, | |
| Type: eventType, | |
| SpecVersion: specVersion, | |
| Topic: msg.Topic, | |
| PubsubName: msg.Metadata[Metadata_key_pubsubName], | |
| } | |
| // set data field | |
| if data, ok := cloudEvent[pubsub.DataBase64Field]; ok && data != nil { | |
| decoded, decodeErr := base64.StdEncoding.DecodeString(data.(string)) | |
| if decodeErr != nil { | |
| log.DefaultLogger.Debugf("unable to base64 decode cloudEvent field data_base64: %s", decodeErr) | |
| return nil, cloudEvent, err | |
| } | |
| envelope.Data = decoded | |
| } else if data, ok := cloudEvent[pubsub.DataField]; ok && data != nil { | |
| envelope.Data = nil | |
| if contenttype.IsStringContentType(envelope.DataContentType) { | |
| envelope.Data = []byte(data.(string)) | |
| } else if contenttype.IsJSONContentType(envelope.DataContentType) { | |
| envelope.Data, _ = a.json.Marshal(data) | |
| } | |
| } | |
| return envelope, cloudEvent, nil | |
| } |
What this PR does:
Support SubscribeTopicEvents API
see #1074
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Summary by CodeRabbit
Release Notes
New Features
Documentation
appcallbackandSubscribeTopicEvents.Tests