chore: NATS broker for micro services#35305
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #35305 +/- ##
===========================================
+ Coverage 69.81% 69.84% +0.02%
===========================================
Files 3297 3298 +1
Lines 119243 119254 +11
Branches 21494 21467 -27
===========================================
+ Hits 83254 83292 +38
+ Misses 32686 32662 -24
+ Partials 3303 3300 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@kody start-review |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| const serviceEvents = new Set<{ | ||
| eventName: keyof EventSignatures; | ||
| listeners: { | ||
| (...args: any[]): void; | ||
| }[]; | ||
| }>(); |
There was a problem hiding this comment.
private serviceEvents = new Set<{
eventName: keyof EventSignatures;
listeners: {
(...args: any[]): void;
}[];
}>();Potential memory leaks due to global serviceEvents Set
This issue appears in multiple locations:
- ee/packages/network-broker/src/NatsBroker.ts: Lines 12-17
Please move serviceEvents to the class instance and implement cleanup in destroyService
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| import { MoleculerBroker } from './MoleculerBroker'; | ||
|
|
||
| const { |
There was a problem hiding this comment.
function validateEnvConfig(config: Record<string, string>) {
const numberFields = ['REQUEST_TIMEOUT', 'HEARTBEAT_INTERVAL', 'HEARTBEAT_TIMEOUT', 'RETRY_FACTOR', 'RETRY_MAX_DELAY'];
const booleanFields = ['RETRY_ENABLED', 'BULKHEAD_ENABLED', 'MS_METRICS'];
for (const field of numberFields) {
if (isNaN(parseInt(config[field]))) {
throw new Error(`Invalid ${field} value: must be a number`);
}
}
for (const field of booleanFields) {
if (![undefined, 'true', 'false'].includes(config[field])) {
throw new Error(`Invalid ${field} value: must be 'true' or 'false'`);
}
}
return config;
}
const config = validateEnvConfig(process.env);Missing function description comments
This issue appears in multiple locations:
- ee/packages/network-broker/src/moleculer.ts: Lines 9-32
- ee/packages/network-broker/src/getInstanceMethods.ts: Lines 3-3
Please add detailed function description comments for better code understanding
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } catch (e) { | ||
| console.error('error', e); | ||
| } |
There was a problem hiding this comment.
} catch (e) {
const error = e instanceof Error ? e : new Error('Unknown error');
console.error('Service method execution failed:', error);
if (msg?.respond) {
msg.respond(TE.encode(EJSON.stringify({ error: error.message })));
}
}Insufficient error handling in service methods
This issue appears in multiple locations:
- ee/packages/network-broker/src/NatsBroker.ts: Lines 87-89
- ee/packages/network-broker/src/NatsBroker.ts: Lines 121-123
Please improve error handling in service methods with proper error propagation
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }; | ||
|
|
||
| export class NetworkBroker implements IBroker { | ||
| export class MoleculerBroker implements IBroker { |
There was a problem hiding this comment.
/**
* Implementation of IBroker using Moleculer service broker
* for handling service communication and event distribution.
*/
export class MoleculerBroker implements IBroker {Inadequate type definitions for error handling
This issue appears in multiple locations:
- ee/packages/network-broker/src/MoleculerBroker.ts: Lines 21-21
- ee/packages/network-broker/src/moleculer.ts: Lines 37-37
Please add proper type definitions for error handling to improve type safety
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
sampaiodiego
force-pushed
the
nats-broker
branch
from
3b2cc8e to
6d69c4a
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sampaiodiego
force-pushed
the
nats-broker
branch
from
6d69c4a to
7b81a9c
Compare
sampaiodiego
force-pushed
the
nats-broker
branch
from
f48f4b5 to
05ab9ff
Compare
🔴 Layne — 2 finding(s)Found 2 issue(s): 1 high, 1 medium. |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <copilot@github.com>
2 participants
https://rocketchat.atlassian.net/browse/RDI-278
Proposed changes (including videos or screenshots)
This is currently a PoC. The idea is to provide a broker for our services architecture that relies only on NATS, so we make use of all its features.
These are tracked items that are still missing:
Issue(s)
Steps to test or reproduce
Further comments
This pull request introduces significant changes to the Rocket.Chat repository, specifically targeting the integration of a NATS broker for microservices. The key modifications include:
Renaming and Refactoring:
NetworkBrokerhas been renamed toMoleculerBrokerin theMoleculerBroker.tsfile. Additionally, the method discovery logic has been extracted into a newgetInstanceMethodsfunction to enhance code organization.NATS Broker Implementation:
NatsBroker.tsfile has been added, implementing a NATS-based broker service. This service includes capabilities for event handling, service creation, and message broadcasting.Test Updates:
MoleculerBroker.test.tshas been updated to replaceNetworkBrokerwithMoleculerBroker, ensuring that the test functionality for service destruction remains intact.Code Organization:
index.tsfile has been simplified by moving broker implementation details to separate files. It now exportsMoleculerBroker,NatsBroker, and thestartBrokerfunction.Moleculer Configuration:
moleculer.tsfile introduces a configuration for the Moleculer service broker, including custom error handling, serialization, and various settings managed through environment variables.API Class Update:
Apiclass inApi.tshas been updated to include a newstartedproperty, along with related changes to manage the broker's start method.These changes aim to enhance the microservices architecture within the Rocket.Chat platform by integrating a robust NATS broker and improving the existing broker infrastructure.