fix: Missing Ingestion Jobs from WebUI Table #679
Conversation
WalkthroughThe pull request introduces a new Changes
Possibly related issues
✨ Finishing Touches
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? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
components/webui/imports/api/ingestion/server/CompressionDbManager.js (2)
26-31: Update JSDoc to reflect parameter changes.The JSDoc comment still references the removed
limitparameter. Please update it to describe thelastUpdateDateparameter.- * Retrieves the last `limit` number of jobs and the ones with the given + * Retrieves jobs updated since the given date and the ones with the given * @param {string} lastUpdateDate
62-64: Consider optimizing the SQL query structure.The current implementation adds the UPDATE_TIME filter to each UNION query, which could impact performance. Consider moving the filter to a WHERE clause after the UNION to improve query efficiency.
- WHERE - _id=${jobId} && - ${COMPRESSION_JOBS_TABLE_COLUMN_NAMES.UPDATE_TIME} >= '${lastUpdateDate}' + WHERE _id=${jobId}Then add after all UNION queries:
WHERE ${COMPRESSION_JOBS_TABLE_COLUMN_NAMES.UPDATE_TIME} >= '${lastUpdateDate}'components/webui/imports/ui/IngestView/panels/IngestionJobs/IngestionJobRow.jsx (1)
109-115: Consider using dayjs for consistent date formatting.The component already uses dayjs for duration calculations. For consistency, consider using it for formatting update_time as well.
- text={(null === job.update_time) ? - "null" : - new Date(job.update_time).toLocaleString()}/> + text={(null === job.update_time) ? + "null" : + dayjs(job.update_time).format('YYYY-MM-DD HH:mm:ss')}/>components/webui/imports/api/ingestion/server/publications.js (2)
23-23: Add a comment explaining the magic number.The constant CONST_FOR_DATE_FORMAT = 19 is used for date string manipulation but its purpose isn't clear. Add a comment explaining that it represents the length of the MySQL datetime format "YYYY-MM-DD HH:MM:SS".
+// Length of MySQL datetime format "YYYY-MM-DD HH:MM:SS" const CONST_FOR_DATE_FORMAT = 19;
48-50: Extract date formatting logic into a helper function.The date string manipulation logic is duplicated. Extract it into a reusable helper function to improve maintainability.
+/** + * Formats a date object to MySQL datetime format + * @param {Date} date + * @return {string} + */ +const formatToMySQLDateTime = (date) => { + return date.toISOString() + .slice(0, CONST_FOR_DATE_FORMAT) + .replace("T", " "); +}; + -let lastUpdateDate = new Date().toISOString() - .slice(0, CONST_FOR_DATE_FORMAT) - .replace("T", " "); +let lastUpdateDate = formatToMySQLDateTime(new Date()); // In refreshCompressionJobs: - const newDate = new Date().toISOString() - .slice(0, CONST_FOR_DATE_FORMAT) - .replace("T", " "); + const newDate = formatToMySQLDateTime(new Date());Also applies to: 107-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
components/clp-py-utils/clp_py_utils/initialize-orchestration-db.py(1 hunks)components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py(5 hunks)components/webui/imports/api/ingestion/constants.js(1 hunks)components/webui/imports/api/ingestion/server/CompressionDbManager.js(3 hunks)components/webui/imports/api/ingestion/server/publications.js(3 hunks)components/webui/imports/ui/IngestView/panels/IngestionJobs/IngestionJobRow.jsx(1 hunks)components/webui/imports/ui/IngestView/panels/IngestionJobs/index.jsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
components/webui/imports/ui/IngestView/panels/IngestionJobs/index.jsx (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/webui/imports/api/ingestion/constants.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/webui/imports/ui/IngestView/panels/IngestionJobs/IngestionJobRow.jsx (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/webui/imports/api/ingestion/server/CompressionDbManager.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/webui/imports/api/ingestion/server/publications.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (4)
components/webui/imports/api/ingestion/constants.js (1)
13-13: LGTM! The new UPDATE_TIME constant is well-placed.The addition follows the existing naming convention and maintains logical grouping with other time-related columns.
components/webui/imports/ui/IngestView/panels/IngestionJobs/index.jsx (1)
50-50: LGTM! The new column header is well-integrated.The "Last Updated" column follows the existing table design pattern with consistent right alignment.
components/clp-py-utils/clp_py_utils/initialize-orchestration-db.py (1)
58-58: LGTM! The update_time column addition is well-structured.The column definition aligns with other timestamp columns in the table and is appropriately positioned with other time-related fields.
components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py (1)
212-219: LGTM! Consistent timestamp updates across all job state changes.The update_time is properly set at all critical points where job status changes:
- When no tasks are created
- When job starts
- When job succeeds
- When job fails
Also applies to: 233-235, 339-339, 350-350
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
components/webui/imports/api/ingestion/server/publications.js (1)
106-112:⚠️ Potential issuePrevent potential race condition in lastUpdateDate updates.
The current implementation has a potential race condition where jobs updated between saving newDate and updating lastUpdateDate could be missed. Consider using a transaction or updating lastUpdateDate before the query.
Additionally, consider adding error handling for date operations to prevent potential runtime errors.
#!/bin/bash # Description: Check for potential race conditions by analyzing job update patterns # Search for concurrent job updates in the codebase rg -A 5 "UPDATE.*COMPRESSION_JOBS.*SET"
🧹 Nitpick comments (2)
components/webui/imports/api/ingestion/server/publications.js (2)
23-24: Document the significance of the magic number.The constant
CONST_FOR_DATE_FORMAT = 19appears to be the length of a MySQL datetime string (YYYY-MM-DD HH:mm:ss). Consider adding a comment explaining this or using a more descriptive constant name.+// Length of MySQL datetime string format 'YYYY-MM-DD HH:mm:ss' const CONST_FOR_DATE_FORMAT = 19;
45-51: Extract date formatting logic into a reusable function.The date formatting logic is duplicated between initialization and updates. Consider extracting it into a helper function to improve maintainability and reduce duplication.
+/** + * Formats a Date object to MySQL datetime string format + * @param {Date} date - The date to format + * @return {string} Formatted date string in 'YYYY-MM-DD HH:mm:ss' format + */ +const formatToMySQLDateTime = (date) => { + return date.toISOString() + .slice(0, CONST_FOR_DATE_FORMAT) + .replace("T", " "); +}; + -let lastUpdateDate = new Date().toISOString() - .slice(0, CONST_FOR_DATE_FORMAT) - .replace("T", " "); +let lastUpdateDate = formatToMySQLDateTime(new Date());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/webui/imports/api/ingestion/server/publications.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
components/webui/imports/api/ingestion/server/publications.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
Outdated
Show resolved
Hide resolved
components/clp-py-utils/clp_py_utils/initialize-orchestration-db.py
Outdated
Show resolved
Hide resolved
|
hey @haiqi96 , can you help review the package scripts? |
components/clp-py-utils/clp_py_utils/initialize-orchestration-db.py
Outdated
Show resolved
Hide resolved
components/webui/imports/ui/IngestView/panels/IngestionJobs/IngestionJobRow.jsx
Outdated
Show resolved
Hide resolved
components/webui/imports/ui/IngestView/panels/IngestionJobs/index.jsx
Outdated
Show resolved
Hide resolved
components/webui/imports/api/ingestion/server/CompressionDbManager.js
Outdated
Show resolved
Hide resolved
components/webui/imports/api/ingestion/server/CompressionDbManager.js
Outdated
Show resolved
Hide resolved
components/webui/imports/api/ingestion/server/CompressionDbManager.js
Outdated
Show resolved
Hide resolved
components/webui/imports/api/ingestion/server/CompressionDbManager.js
Outdated
Show resolved
Hide resolved
components/webui/imports/api/ingestion/server/CompressionDbManager.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/webui/imports/api/ingestion/server/publications.js (1)
89-98: Consider using max retrieval_time for more accurate synchronization.While using retrieval_time from the database is good, using the first job's timestamp might miss updates if multiple jobs have the same retrieval_time. Consider using the maximum retrieval_time from all jobs.
- lastUpdateTimestampSeconds = jobs[0].retrieval_time; + lastUpdateTimestampSeconds = Math.max(...jobs.map(job => job.retrieval_time));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/clp-py-utils/clp_py_utils/initialize-orchestration-db.py(1 hunks)components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py(1 hunks)components/webui/imports/api/ingestion/server/CompressionDbManager.js(1 hunks)components/webui/imports/api/ingestion/server/publications.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/clp-py-utils/clp_py_utils/initialize-orchestration-db.py
- components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
🧰 Additional context used
📓 Path-based instructions (2)
components/webui/imports/api/ingestion/server/publications.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/webui/imports/api/ingestion/server/CompressionDbManager.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (3)
components/webui/imports/api/ingestion/server/CompressionDbManager.js (2)
23-28: LGTM! Clear and accurate docstring.The docstring effectively communicates the function's purpose, parameter type, and return value structure.
29-47: LGTM! Well-implemented timestamp-based filtering.The implementation effectively:
- Uses FROM_UNIXTIME() for timestamp comparison
- Includes retrieval_time for accurate synchronization
- Maintains consistent ordering with ORDER BY
- Properly parameterizes the query for security
components/webui/imports/api/ingestion/server/publications.js (1)
39-42: LGTM! Well-documented timestamp variable.The variable declaration includes proper JSDoc typing and uses a clear, descriptive name that includes the unit.
components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
Outdated
Show resolved
Hide resolved
components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
Outdated
Show resolved
Hide resolved
components/webui/imports/api/ingestion/server/CompressionDbManager.js
Outdated
Show resolved
Hide resolved
components/webui/imports/api/ingestion/server/CompressionDbManager.js
Outdated
Show resolved
Hide resolved
components/webui/imports/api/ingestion/server/CompressionDbManager.js
Outdated
Show resolved
Hide resolved
components/webui/imports/api/ingestion/server/CompressionDbManager.js
Outdated
Show resolved
Hide resolved
components/clp-py-utils/clp_py_utils/initialize-orchestration-db.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/webui/imports/api/ingestion/server/publications.js (1)
39-43: Global variable usage for timestamp is acceptable with single-threaded logic.The definition of
lastUpdateTimestampSecondsin the global scope works under the assumption that the refresh operation does not run concurrently. If concurrency is introduced in the future, consider adopting a more robust synchronization approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/clp-py-utils/clp_py_utils/initialize-orchestration-db.py(2 hunks)components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py(1 hunks)components/webui/imports/api/ingestion/server/CompressionDbManager.js(1 hunks)components/webui/imports/api/ingestion/server/publications.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
- components/clp-py-utils/clp_py_utils/initialize-orchestration-db.py
🧰 Additional context used
📓 Path-based instructions (2)
components/webui/imports/api/ingestion/server/publications.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/webui/imports/api/ingestion/server/CompressionDbManager.js (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (5)
components/webui/imports/api/ingestion/server/CompressionDbManager.js (3)
23-25: Clarify parameter usage in JSDoc.The JSDoc now correctly indicates that the method retrieves all compression jobs updated on or after the given timestamp in seconds. Consider making it explicit that it uses UNIX time in seconds, if that is the underlying assumption.
29-29: Method signature update looks consistent.Switching to a single parameter for timestamp-based filtering simplifies the query logic and aligns with the PR objective to retrieve all updated jobs.
31-44:⚠️ Potential issuePotentially incorrect subtraction of 1 using modulo arithmetic.
FROM_UNIXTIME(${lastUpdateTimestampSeconds}) - 1could be interpreted in MySQL as subtracting one day or performing an unintentional numeric conversion, instead of subtracting one second. If your intention is to subtract one second from the resulting datetime, please consider using theDATE_SUBfunction:-WHERE ${COMPRESSION_JOBS_TABLE_COLUMN_NAMES.UPDATE_TIME} >= - FROM_UNIXTIME(${lastUpdateTimestampSeconds}) -1 +WHERE ${COMPRESSION_JOBS_TABLE_COLUMN_NAMES.UPDATE_TIME} >= + DATE_SUB(FROM_UNIXTIME(${lastUpdateTimestampSeconds}), INTERVAL 1 SECOND)Likely invalid or redundant comment.
components/webui/imports/api/ingestion/server/publications.js (2)
89-89: Good removal of limit-based argument.Passing
lastUpdateTimestampSecondstogetCompressionJobsbetter aligns with the new timestamp-based retrieval model.
92-98: Validate pecking order for retrieval_time.When updating
lastUpdateTimestampSecondswithjobs[0].retrieval_time, ensure the job at index 0 is guaranteed to have the maximum retrieval time. If another entry has a later retrieval_time but a smaller_id, it might be missed on subsequent calls. Consider computing the maximum retrieval_time across all returned jobs, if necessary.
Description
#667
When ingestion jobs are submitted at a fast enough pace the ingestion table in the UI would be missing some of the submitted jobs. The ingestion table would request the most recent 5 jobs when looking for jobs and if job came quicker in the previous time period, not all jobs would be reflected in the UI.
FIX:
Validation performed
Submitted increasingly large number soft background compression jobs and made sure that they were all reflected in the table and that the updated to the status in the ingestion table occur accordingly.
Summary by CodeRabbit
Release Notes
New Features
update_timecolumn to track the last update timestamp for compression jobs.Improvements
These changes enhance the system's efficiency in tracking and retrieving compression job information.