refactor(webui): Migrate extract stream route and plugin to new Fastify architecture. #1065
Conversation
WalkthroughThis change refactors the server-side handling of stream file extraction and query job management by replacing the legacy Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Route as /api/stream-files/extract
participant StreamFileManager
participant QueryJobDbManager
participant MongoDB
participant S3Manager
Client->>API_Route: POST extract (dataset, extractJobType, logEventIdx, streamId)
API_Route->>StreamFileManager: getExtractedStreamFileMetadata(streamId, logEventIdx)
StreamFileManager->>MongoDB: Find stream file metadata
alt Metadata Found
StreamFileManager-->>API_Route: Return metadata
else Metadata Not Found
StreamFileManager->>QueryJobDbManager: submitAndWaitForExtractStreamJob(...)
QueryJobDbManager->>MySQL: Insert job, poll for completion
QueryJobDbManager-->>StreamFileManager: Job complete
StreamFileManager->>MongoDB: Find stream file metadata
StreamFileManager-->>API_Route: Return metadata or null
end
alt S3Manager available
API_Route->>S3Manager: getPresignedUrl(path)
S3Manager-->>API_Route: S3 URL
API_Route-->>Client: Respond with metadata (S3 URL)
else
API_Route-->>Client: Respond with metadata (local path)
end
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}Instructions used from: Sources: 🧠 Learnings (2)📓 Common learningscomponents/webui/server/src/app.ts (3)🔇 Additional comments (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
davemarco
changed the title
davemarco
changed the title
|
@hoophalab , i merged main into this and some smaller changes |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
components/webui/client/src/api/stream-files/index.ts(1 hunks)components/webui/client/src/ui/QueryStatus.tsx(1 hunks)components/webui/client/vite.config.ts(1 hunks)components/webui/server/src/app.ts(1 hunks)components/webui/server/src/fastify-v2/app.ts(1 hunks)components/webui/server/src/fastify-v2/plugins/app/QueryJobDbManager/index.ts(1 hunks)components/webui/server/src/fastify-v2/plugins/app/QueryJobDbManager/typings.ts(1 hunks)components/webui/server/src/fastify-v2/plugins/app/StreamFileManager.ts(1 hunks)components/webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts(0 hunks)components/webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/typings.ts(0 hunks)components/webui/server/src/fastify-v2/routes/api/search/index.ts(6 hunks)components/webui/server/src/fastify-v2/routes/api/search/typings.ts(1 hunks)components/webui/server/src/fastify-v2/routes/api/search/utils.ts(2 hunks)components/webui/server/src/fastify-v2/routes/api/stream-files/index.ts(1 hunks)components/webui/server/src/fastify-v2/schemas/stream-files.ts(1 hunks)components/webui/server/src/plugins/DbManager.ts(0 hunks)components/webui/server/src/routes/query.ts(0 hunks)
💤 Files with no reviewable changes (4)
- components/webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/typings.ts
- components/webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
- components/webui/server/src/routes/query.ts
- components/webui/server/src/plugins/DbManager.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧠 Learnings (13)
📓 Common learnings
Learnt from: gibber9809
PR: y-scope/clp#504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/clp/FileCompressor.hpp:58-78
Timestamp: 2024-10-24T14:25:17.978Z
Learning: When reviewing legacy code refactors, avoid suggesting changes that would extend the scope of the PR.
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/client/src/api/stream-files/index.ts (1)
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/client/src/ui/QueryStatus.tsx (2)
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:35-41
Timestamp: 2024-11-19T19:52:43.429Z
Learning: For internal APIs in `components/log-viewer-webui/client/src/api/query.js`, runtime validation of parameters may not be necessary since the APIs are not exposed to end users, and JsDoc type annotations may be sufficient.
components/webui/server/src/fastify-v2/app.ts (3)
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.164Z
Learning: When using Fastify's `app.close()` method, always wrap it in a try/catch block to properly handle shutdown errors. The close() method returns a Promise that can reject if errors occur during the shutdown process (especially with plugins using onClose hooks), and unhandled rejections could cause the application to hang.
components/webui/client/vite.config.ts (1)
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/server/src/fastify-v2/routes/api/search/utils.ts (1)
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/server/src/fastify-v2/routes/api/search/typings.ts (1)
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/server/src/fastify-v2/schemas/stream-files.ts (1)
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/server/src/app.ts (4)
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.164Z
Learning: When using Fastify's `app.close()` method, always wrap it in a try/catch block to properly handle shutdown errors. The close() method returns a Promise that can reject if errors occur during the shutdown process (especially with plugins using onClose hooks), and unhandled rejections could cause the application to hang.
Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.164Z
Learning: In Fastify applications, the `app.close()` method returns a Promise that can reject if errors occur during shutdown. Always wrap it in a try/catch block to properly handle shutdown errors and set the process exit code to prevent the application from hanging.
components/webui/server/src/fastify-v2/routes/api/stream-files/index.ts (2)
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/server/src/fastify-v2/routes/api/search/index.ts (1)
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/server/src/fastify-v2/plugins/app/QueryJobDbManager/index.ts (1)
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
components/webui/server/src/fastify-v2/plugins/app/StreamFileManager.ts (3)
Learnt from: davemarco
PR: y-scope/clp#1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
🧬 Code Graph Analysis (3)
components/webui/server/src/fastify-v2/routes/api/search/utils.ts (1)
components/webui/server/src/fastify-v2/routes/api/search/typings.ts (1)
UpdateSearchSignalWhenJobsFinishProps(50-50)
components/webui/server/src/fastify-v2/schemas/stream-files.ts (1)
components/webui/client/src/typings/query.ts (1)
QUERY_JOB_TYPE(94-94)
components/webui/server/src/fastify-v2/routes/api/search/index.ts (1)
components/webui/client/src/typings/query.ts (1)
QUERY_JOB_TYPE(94-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: conventional-commits
🔇 Additional comments (15)
components/webui/client/src/ui/QueryStatus.tsx (1)
13-13: Import path correctly updated to reflect API restructuring.The import change aligns with the backend migration from
/query/extract-streamto/api/stream-files/extractendpoint.components/webui/client/src/api/stream-files/index.ts (1)
41-41: API endpoint correctly updated to match new backend route.The endpoint change from
/query/extract-streamto/api/stream-files/extractaligns with the Fastify architecture migration.components/webui/client/vite.config.ts (1)
25-26: Proxy configuration comment updated to reflect multiple targets.The comment change from "target" to "targets" correctly reflects the current proxy configuration structure.
components/webui/server/src/fastify-v2/app.ts (1)
42-42: FastifyV1App registration simplified by removing database configuration options.The removal of
sqlDbUserandsqlDbPassoptions aligns with the migration away from the legacyDbManagerplugin to the new architecture.components/webui/server/src/fastify-v2/plugins/app/QueryJobDbManager/typings.ts (1)
1-6: Well-defined polling interval constant with appropriate value.The 500ms interval strikes a good balance between responsiveness and performance for job status polling.
components/webui/server/src/fastify-v2/routes/api/search/utils.ts (1)
60-78: LGTM! Consistent parameter renaming.The renaming from
queryJobsDbManagertoqueryJobDbManageris correctly applied throughout the function, including the documentation, parameter declaration, and method calls.components/webui/server/src/fastify-v2/routes/api/search/typings.ts (1)
33-33: Type definition correctly updated.The property rename aligns with the new
QueryJobDbManagerplugin name and maintains consistency with the implementation in utils.ts.components/webui/server/src/app.ts (1)
21-23: Clean refactoring removing database dependencies.The simplified plugin signature correctly reflects the removal of database configuration, aligning with the new modular architecture where database management is handled by dedicated plugins.
components/webui/server/src/fastify-v2/schemas/stream-files.ts (2)
11-18: Good handling of null values in union type.The comment correctly explains why
Type.Null()must come beforeType.String()to prevent unwanted type coercion. This ensures proper validation of null dataset values.
27-32: Inconsistent naming convention in schema.The
stream_idproperty uses snake_case while other properties (begin_msg_ix,end_msg_ix) use snake_case andpathuses lowercase. Consider standardizing the naming convention across all properties for consistency.⛔ Skipped due to learnings
Learnt from: junhaoliao PR: y-scope/clp#596 File: components/log-viewer-webui/client/src/api/query.js:16-23 Timestamp: 2024-11-21T15:51:33.203Z Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.Learnt from: haiqi96 PR: y-scope/clp#620 File: components/core/src/clp/clo/clo.cpp:198-198 Timestamp: 2024-12-02T22:36:17.847Z Learning: The existing references to `file_split_id` in the codebase are acceptable and do not need to be changed.components/webui/server/src/fastify-v2/routes/api/stream-files/index.ts (3)
36-38: Good adherence to coding guidelines.The condition correctly uses
false === <expression>pattern as specified in the coding guidelines.
45-72: Robust error handling with appropriate checks.The implementation properly handles the extraction workflow with multiple validation points and clear error messages. The double-check pattern after job completion ensures data consistency.
74-81: Well-implemented conditional path handling.The code correctly checks for the S3 manager decorator before use and provides appropriate fallback for local file paths.
components/webui/server/src/fastify-v2/routes/api/search/index.ts (1)
88-101: LGTM! Clean migration to unified job submission interface.The refactoring properly uses the new
submitJobmethod with the appropriate job type enum. The aggregation job correctly includes theaggregation_configparameter.components/webui/server/src/fastify-v2/plugins/app/QueryJobDbManager/index.ts (1)
114-124: LGTM! Proper status checking and error handling.The code correctly checks for terminal states and provides clear error messages for different failure scenarios. Good use of the
false ===pattern as per coding guidelines.
components/webui/server/src/fastify-v2/plugins/app/QueryJobDbManager/index.ts
Show resolved
Hide resolved
components/webui/server/src/fastify-v2/plugins/app/QueryJobDbManager/index.ts
Show resolved
Hide resolved
hoophalab
left a comment
hoophalab
left a comment
There was a problem hiding this comment.
lgtm. just some nitpicks
Validations:
- run
clp-textandclp-json - compress
hive-24hrandpostgresqlrespectively - query worked
- query can be successfully cancelled
- stream files extracted correctly
|
|
||
| return streamMetadata; |
There was a problem hiding this comment.
Can we add an explicit type conversion here?
The return type of getExtractedStreamFileMetadata is StreamFileMongoDocument, but the return type is typeof StreamFileMetadataSchema. It took me some time to confirm StreamFileMongoDocument is a superset of typeof StreamFileMetadataSchema. The old code doesn't have this problem because its response is not type safe at all xD
| return streamMetadata; | |
| return streamMetadata as Static<typeof StreamFileMetadataSchema>; |
where import {Static} from "@sinclair/typebox";
- Do not commit suggestion directly as the syntax could be incorrect
There was a problem hiding this comment.
I refactored the code to use the same base type for StreamFileMetadataSchema and StreamFileMongoDocument to make things clearer and prevent duplication.
We generally try and avoid type conversion, and I dont think neccesary here. Typebox/Fastify/Typescript will verify that the response matches the declared typebox type, like if you didnt return StreamFileMetadataSchema here, there would be a built error and linter will complain. Like there is already a type assertion in effect, so I dont believe we should cast it.
There was a problem hiding this comment.
If that's the method we are going to take, then I would recommend rename StreamFileMongoDocument as StreamFileMetadata (or StreamFileMetadataSchemaType or something similar).
| }: SubmitExtractStreamJobProps): Promise<AxiosResponse<ExtractStreamResp>> => { | ||
| return await axios.post( | ||
| "/query/extract-stream", | ||
| "/api/stream-files/extract", |
There was a problem hiding this comment.
qq: should this be considered as a breaking change refactor(webui)!: or it is not because this is an internal api?
There was a problem hiding this comment.
Our policy is to mark breaking changes for changes that would be visible to users (since that requires that we bump the relevant version number). So if this is purely internal to this repo, I would say it's not breaking.
|
@hoophalab ready for rereview |
hoophalab
left a comment
hoophalab
left a comment
There was a problem hiding this comment.
lgtm
just a friendly reminder to remove the duplicate "Marco" and other unrelated authors from the co-author list when you merge
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
deferring to @hoophalab 's review
There was a problem hiding this comment.
qq: what's the plan for the static server and s3manager plugins in this file? i.e., do we plan to migrate them as well in the near future, or to keep it in this "v1" plugin?
There was a problem hiding this comment.
i already put up the s3 manager pr. The static i will do later. we are going to delete the v1 plugin
4 participants
This PR is dependent on #1027, #1028 being merged
Description
PR moves extract stream route and plugin into new fastify architecture. This is a chain of PRs which started with #1027
There was alot of duplication between the plugin that submitted search jobs, and the old extract stream plugin (they both submit jobs to the same mysql database), so i completely refactored so they share more code. As a result, the PR is not trivial.
Checklist
breaking change.
Validation performed
tested streams with clp and clp-s
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores