refactor(core): Flatten Redis pubsub class hierarchy (no-changelog) #10616
Conversation
n8n-assistant
bot
added
core
ivov
force-pushed
the
flatten-redis-hierarchy
branch
from
e88ea1f to
15a2a16
Compare
| getClient() { | ||
| return this.client; | ||
| } |
There was a problem hiding this comment.
Do we need to expose the client publicly?
There was a problem hiding this comment.
For testing only, it's an open discussion on what's best practice for this.
There was a problem hiding this comment.
Yeah I keep wavering between the both options. On one hand it's nice to test certain things that require private parts, but then again tests should test the public interface or it's too tied to implementation details 🤷 No need to change this now
packages/cli/src/services/orchestration/main/orchestration.handler.main.service.ts
Outdated
Show resolved
Hide resolved
packages/cli/src/services/orchestration/webhook/orchestration.handler.webhook.service.ts
Outdated
Show resolved
Hide resolved
| @@ -29,7 +17,6 @@ export abstract class OrchestrationHandlerService { | |||
| } | |||
|
|
|||
| async shutdown() { | |||
| await this.redisSubscriber?.destroy(); | |||
There was a problem hiding this comment.
Who takes care of destroying the subscriber now?
There was a problem hiding this comment.
Here:
n8n/packages/cli/src/services/orchestration.service.ts
Lines 84 to 94 in 9543d88
packages/cli/src/services/orchestration/worker/orchestration.handler.worker.service.ts
Outdated
Show resolved
Hide resolved
|
Once there are no more code changes, I'll do another round of manual testing. |
n8n
|
||||||||||||||||||||||||||||
| Project |
n8n
|
| Branch Review |
flatten-redis-hierarchy
|
| Run status |
|
| Run duration | 04m 39s |
| Commit |
|
| Committer | Iván Ovejero |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
430
|
| View all changes introduced in this branch ↗︎ | |
|
✅ All Cypress E2E specs passed |
|
Retested manually that these are still sent and received:
@tomi If you can please reapprove 🙏🏻 |
Will do 👍 You can use the re-request review to notify me:
|
|
✅ All Cypress E2E specs passed |
- Move `/redis` under scaling mode - Move channel constants to `scaling/constants.ts` - Delete `redis-service-base-classes.ts` - unused since #10616 - Delete `WorkerJobStatusSummary` - unused - Move pubsub map to `pubsub.types.ts` - Rename `PubSubMessage` to `JobReport` for consistency
|
Got released with |
To set up a publisher and subscriber for scaling mode, we currently rely on six interrelated classes:
RedisService,RedisServiceBase,RedisServiceBaseSender,RedisServiceBaseReceiver,RedisServicePubSubPublisher,RedisServicePubSubSubscriberThis PR simplifies this six-class setup into two small classes
PublisherandSubscriber, tightens typings, adds documentation, removes dead code, and deletes all six original classes.Further improvements are marked as TODOs, outside the scope of this PR.
Follow-up to: #10566