bito-code-review[bot] commented on code in PR #43625:
URL: https://github.com/apache/superset/pull/43625#discussion_r3876279842
##########
superset-websocket/utils/loadtest.js:
##########
@@ -22,38 +22,32 @@ const { randomUUID } = require('crypto');
const redis = new Redis(config.redis);
const numClients = 256;
-const globalEventStreamName = `${config.redisStreamPrefix}full`;
+
+// The Pub/Sub channels the server tails; a fixed wire-protocol contract with
the
+// Superset producer, mirrored from superset-websocket/src/index.ts.
+const entityChangesChannel = 'entity-changes:task';
+const taskStatusChannel = 'task-status';
function pushData() {
- for (let i = 0; i < numClients; i++) {
- const channelId = String(i);
- const streamId = `${config.redisStreamPrefix}${channelId}`;
- const data = {
- channel_id: channelId,
- job_id: randomUUID(),
- status: 'pending',
- };
+ const taskId = randomUUID();
- // push to channel stream
- redis
- .xadd(streamId, 'MAXLEN', 1000, '*', 'data', JSON.stringify(data))
- .then(resp => {
- console.log('stream response', resp);
- });
+ // Tier 1: one broadcast entity-change nudge, carrying only opaque ids.
+ redis.publish(
+ entityChangesChannel,
+ JSON.stringify({ entity_type: 'task', id: taskId }),
+ );
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unhandled publish rejections</b></div>
<div id="fix">
`redis.publish()` returns a Promise. The previous `xadd` chain had
`.then(...)` handlers; the new code drops them. On a 1-second interval with 257
publishes per tick, any Redis blip will surface as unhandled rejections and
(Node ≥15) terminate the process. Attach `.catch(err => console.error(err))` to
each publish, or `await Promise.all(...)` inside `pushData`.
</div>
</div>
<small><i>Code Review Run #4239b7</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-websocket/spec/index.test.ts:
##########
@@ -251,14 +449,27 @@ describe('server', () => {
);
});
+ test('drops a guest subscriber whose key is not namespaced', () => {
+ const ws = new wsMock('localhost');
+ const send = vi.spyOn(ws, 'send');
+ trackSocket('abc', ws);
+
+ server.routeRedisMessage(
+ 'task-status',
+ JSON.stringify({
+ task_id: 'abc',
+ status: 'success',
+ subscribers: [{ principal_type: 'guest', sub: 'abc' }],
+ }),
+ );
+
+ expect(send).not.toHaveBeenCalled();
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Misleading test setup</b></div>
<div id="fix">
The test name says "drops a guest subscriber whose key is not namespaced,"
but `trackSocket('abc', ws)` registers a **user** identity (since `'abc'`
doesn't start with `'guest:'`), not a guest socket. The assertion passes
regardless of the tracked socket because `principalChannel('guest', 'abc')`
returns `null` in `sendTaskStatusToSubscribers` before any `sendToChannel`
call. This duplicates the existing `principalChannel` test at line 333 and
doesn't exercise the routing path its name implies.
</div>
</div>
<small><i>Code Review Run #4239b7</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
