feat(package): Add dataset-manager scripts to support listing datasets, and deleting them entirely. - #1144
Conversation
WalkthroughAdds package and native dataset-manager CLIs (list/del) with containerized execution to remove dataset archives and per-dataset metadata; expands S3 utilities (region/auth client creation, batched prefix deletion); adds archive-manager and compression-tasks constants; updates compression scheduler to fetch existing datasets internally; adds Bash wrapper and an IAM doc policy change. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BashScript
participant PackageCLI
participant Container
participant NativeCLI
participant S3
participant MetadataDB
User->>BashScript: run dataset-manager.sh [args]
BashScript->>PackageCLI: python -m clp_package_utils.scripts.dataset_manager [args]
PackageCLI->>Container: write container config, prepare mounts, start container
Container->>NativeCLI: execute native dataset_manager (list|del)
NativeCLI->>MetadataDB: connect, fetch dataset info
alt del with S3 archives
NativeCLI->>S3: s3_delete_by_key_prefix(region, bucket, prefix)
else del with filesystem archives
NativeCLI->>NativeCLI: validate and remove archive directory
end
NativeCLI->>MetadataDB: drop per-dataset tables, remove dataset row
NativeCLI-->>Container: exit code
Container-->>PackageCLI: result
PackageCLI-->>User: exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (7)📚 Learning: 2025-08-13T14:48:49.020ZApplied to files:
📚 Learning: 2025-07-03T12:58:18.407ZApplied to files:
📚 Learning: 2025-07-03T20:10:43.789ZApplied to files:
📚 Learning: 2025-01-23T17:08:55.566ZApplied to files:
📚 Learning: 2024-11-15T16:21:52.122ZApplied to files:
📚 Learning: 2025-08-13T15:07:37.767ZApplied to files:
📚 Learning: 2025-08-13T15:07:37.767ZApplied to files:
⏰ 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)
🔇 Additional comments (6)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
| datasets_to_delete.append(dataset) | ||
|
|
||
| if 0 == len(datasets_to_delete): | ||
| logger.warning("No dataset will be deleted...") |
There was a problem hiding this comment.
Any suggestion on the warning message?
We may not need this message if we decide
- force all input dataset to be valid.
- have a more specific error message when --all is used by no dataset exists.
| # Add trailing '/' to avoid deleting other datasets with similar prefixes | ||
| if not dataset_archive_storage_dir.endswith("/"): |
There was a problem hiding this comment.
Alternatively, we can add the "/" to the dataset metadata (which is better imo)
haiqi96
changed the title
dataset-manager scripts to support dataset manangement.dataset-manager scripts to support datasets manangement.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py(1 hunks)components/clp-py-utils/clp_py_utils/s3_utils.py(4 hunks)components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py(2 hunks)components/package-template/src/sbin/admin-tools/dataset-manager.sh(1 hunks)docs/src/user-guide/guides-using-object-storage/object-storage-config.md(1 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1050
File: components/clp-package-utils/clp_package_utils/scripts/search.py:100-106
Timestamp: 2025-07-03T20:10:43.789Z
Learning: In the current CLP codebase implementation, dataset validation using validate_dataset() is performed within the native scripts (like clp_package_utils/scripts/native/search.py) rather than at the wrapper script level, where the native scripts handle their own parameter validation.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1050
File: components/clp-package-utils/clp_package_utils/scripts/search.py:100-106
Timestamp: 2025-07-03T20:10:43.789Z
Learning: In the current CLP codebase implementation, dataset validation using validate_dataset() is performed within the native scripts (like clp_package_utils/scripts/native/search.py) rather than at the wrapper script level, where the native scripts handle their own parameter validation.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1036
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py:204-211
Timestamp: 2025-07-03T12:58:18.407Z
Learning: In the CLP codebase, the validate_and_cache_dataset function in components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py uses in-place updates of the existing_datasets set parameter rather than returning a new set, as preferred by the development team.
📚 Learning: s3 roles provided may not have permission to perform `head_bucket` and `delete_object` operations; v...
Learnt from: haiqi96
PR: y-scope/clp#634
File: components/clp-py-utils/clp_py_utils/s3_utils.py:11-16
Timestamp: 2024-12-12T19:20:59.778Z
Learning: S3 roles provided may not have permission to perform `head_bucket` and `delete_object` operations; verification logic should avoid using these methods.
Applied to files:
docs/src/user-guide/guides-using-object-storage/object-storage-config.mdcomponents/clp-py-utils/clp_py_utils/s3_utils.py
📚 Learning: in the current clp codebase implementation, dataset validation using validate_dataset() is performed...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1050
File: components/clp-package-utils/clp_package_utils/scripts/search.py:100-106
Timestamp: 2025-07-03T20:10:43.789Z
Learning: In the current CLP codebase implementation, dataset validation using validate_dataset() is performed within the native scripts (like clp_package_utils/scripts/native/search.py) rather than at the wrapper script level, where the native scripts handle their own parameter validation.
Applied to files:
components/package-template/src/sbin/admin-tools/dataset-manager.shcomponents/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/dataset_manager.pycomponents/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
📚 Learning: in the clp codebase, the validate_and_cache_dataset function in components/clp-py-utils/clp_py_utils...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1036
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py:204-211
Timestamp: 2025-07-03T12:58:18.407Z
Learning: In the CLP codebase, the validate_and_cache_dataset function in components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py uses in-place updates of the existing_datasets set parameter rather than returning a new set, as preferred by the development team.
Applied to files:
components/package-template/src/sbin/admin-tools/dataset-manager.shcomponents/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/dataset_manager.pycomponents/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
📚 Learning: for wrapper scripts in the `components/package-template/src/sbin/` directory, keep them simple and a...
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/package-template/src/sbin/del-archives.sh:6-9
Timestamp: 2024-11-15T16:28:08.644Z
Learning: For wrapper scripts in the `components/package-template/src/sbin/` directory, keep them simple and avoid adding additional validation code.
Applied to files:
components/package-template/src/sbin/admin-tools/dataset-manager.sh
📚 Learning: when reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note th...
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
components/package-template/src/sbin/admin-tools/dataset-manager.sh
📚 Learning: in clp installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistenc...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Applied to files:
components/package-template/src/sbin/admin-tools/dataset-manager.sh
📚 Learning: the validate_dataset function in components/clp-package-utils/clp_package_utils/general.py is design...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1036
File: components/clp-package-utils/clp_package_utils/general.py:564-579
Timestamp: 2025-06-28T07:10:47.295Z
Learning: The validate_dataset function in components/clp-package-utils/clp_package_utils/general.py is designed to be called once upon function startup for dataset validation, not repeatedly during execution, making caching optimizations unnecessary.
Applied to files:
components/package-template/src/sbin/admin-tools/dataset-manager.shcomponents/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/dataset_manager.pycomponents/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
📚 Learning: for installation scripts in the clp project, prefer explicit error handling over automatic dependenc...
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Applied to files:
components/package-template/src/sbin/admin-tools/dataset-manager.sh
📚 Learning: in the clp codebase, the get_orig_file_id function signature was changed after a recent merge to no ...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1004
File: components/clp-package-utils/clp_package_utils/scripts/native/decompress.py:139-144
Timestamp: 2025-06-24T08:54:14.438Z
Learning: In the CLP codebase, the get_orig_file_id function signature was changed after a recent merge to no longer accept a dataset parameter, making previous suggestions that reference this parameter invalid.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/dataset_manager.pycomponents/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
📚 Learning: the column metadata table (created by `_create_column_metadata_table`) is only needed for dataset-sp...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#868
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py:141-144
Timestamp: 2025-05-05T16:32:55.163Z
Learning: The column metadata table (created by `_create_column_metadata_table`) is only needed for dataset-specific workflows in `CLP_S` and is obsolete for non-dataset workflows.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.pycomponents/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
📚 Learning: in `clp_package_utils/scripts/native/del_archives.py`, when deleting archives, the `archive` variabl...
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:104-110
Timestamp: 2024-11-15T16:21:52.122Z
Learning: In `clp_package_utils/scripts/native/del_archives.py`, when deleting archives, the `archive` variable retrieved from the database is controlled and is always a single string without path components. Therefore, it's acceptable to skip additional validation checks for directory traversal in this context.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: in the clp-package compression flow, path validation and error handling is performed at the schedule...
Learnt from: haiqi96
PR: y-scope/clp#651
File: components/clp-package-utils/clp_package_utils/scripts/compress.py:0-0
Timestamp: 2025-01-16T16:58:43.190Z
Learning: In the clp-package compression flow, path validation and error handling is performed at the scheduler level rather than in the compress.py script to maintain simplicity and avoid code duplication.
Applied to files:
components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
📚 Learning: in clp schedulers (compression and query), runtime dataset validation is required; the helper valida...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1036
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py:188-212
Timestamp: 2025-07-03T12:59:46.638Z
Learning: In CLP schedulers (compression and query), runtime dataset validation is required; the helper validate_and_cache_dataset keeps a local cache and only queries the DB on cache misses because dataset additions are rare.
Applied to files:
components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
📚 Learning: the s3 service in the clp codebase only supports aws s3, where region_code is mandatory. other s3-li...
Learnt from: haiqi96
PR: y-scope/clp#651
File: components/job-orchestration/job_orchestration/scheduler/job_config.py:29-39
Timestamp: 2025-01-15T16:36:48.932Z
Learning: The S3 service in the clp codebase only supports AWS S3, where region_code is mandatory. Other S3-like services are not supported.
Applied to files:
components/clp-py-utils/clp_py_utils/s3_utils.py
📚 Learning: the s3_put api in clp_py_utils.s3_utils internally handles timeout and retry mechanisms for s3 opera...
Learnt from: haiqi96
PR: y-scope/clp#662
File: components/job-orchestration/job_orchestration/executor/query/extract_stream_task.py:167-186
Timestamp: 2025-01-13T21:18:54.629Z
Learning: The s3_put API in clp_py_utils.s3_utils internally handles timeout and retry mechanisms for S3 operations.
Applied to files:
components/clp-py-utils/clp_py_utils/s3_utils.py
📚 Learning: for s3 urls without region specifications (legacy global endpoints), either assign a default region ...
Learnt from: haiqi96
PR: y-scope/clp#852
File: components/clp-package-utils/clp_package_utils/scripts/native/compress.py:151-160
Timestamp: 2025-04-25T20:46:20.140Z
Learning: For S3 URLs without region specifications (legacy global endpoints), either assign a default region (us-east-1) or throw a clear error message requiring region specification in the URL. This addresses validation issues in components like S3InputConfig that require a non-nullable region string.
Applied to files:
components/clp-py-utils/clp_py_utils/s3_utils.py
🧬 Code Graph Analysis (2)
components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py (1)
components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py (1)
fetch_existing_datasets(184-196)
components/clp-py-utils/clp_py_utils/s3_utils.py (1)
components/clp-py-utils/clp_py_utils/clp_config.py (2)
AwsAuthentication(338-367)AwsAuthType(57-61)
🔇 Additional comments (20)
docs/src/user-guide/guides-using-object-storage/object-storage-config.md (1)
69-81: LGTM! Necessary IAM permission for dataset deletion.The addition of
s3:ListBucketpermission with prefix condition aligns perfectly with the new dataset manager's S3 deletion functionality introduced in this PR. The permission structure is consistent with the compression configuration example and properly restricts access to the specified key prefix.components/package-template/src/sbin/admin-tools/dataset-manager.sh (1)
1-9: LGTM! Clean wrapper script following CLP conventions.The script correctly sets up the Python environment and delegates to the Python module. The use of
readlink -ffor absolute path resolution and"$@"for argument forwarding follows established patterns in the CLP codebase.components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (7)
35-40: LGTM! Clean list implementation.The function correctly handles dataset listing with appropriate logging and simple iteration over the datasets dictionary.
42-63: LGTM! Proper database query with parameterized statements.The function correctly uses parameterized queries to avoid SQL injection and properly handles the database connection lifecycle with context managers.
120-126: LGTM! Correct table deletion order.The table removal order properly handles foreign key constraints by deleting dependent tables first (column_metadata, files, archive_tags, tags) before the main archives table.
132-133: LGTM! Proper use of parameterized queries.The table drops use string formatting which is safe here since table names are controlled, and the dataset deletion uses parameterized queries to prevent SQL injection.
147-150: LGTM! Proper S3 prefix handling.Adding the trailing slash prevents accidental deletion of datasets with similar prefixes (e.g., "logs" vs "logs-backup"). This is a critical security consideration for S3 operations.
163-167: LGTM! Strong path traversal protection.The use of
Path.resolve()andis_relative_to()provides robust protection against directory traversal attacks, ensuring deletions are confined to the configured archive directory.
169-177: LGTM! Defensive file system checks.The function properly validates that the path exists and is a directory before attempting deletion, with appropriate debug logging for edge cases.
components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py (2)
171-175: LGTM! Improved function encapsulation.Moving the
fetch_existing_datasetscall inside the function improves encapsulation and makes the function more self-contained. The logic remains identical while simplifying the interface.
198-199: LGTM! Updated comment reflects new dataset management capabilities.The comment correctly clarifies the assumption about dataset deletion timing, which is relevant given the new dataset management tools introduced in this PR.
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (5)
92-95: LGTM! Proper storage engine validation.Correctly restricts dataset operations to CLP_S storage engine, which is the only one that supports datasets.
101-106: LGTM! Proper mutual exclusivity validation.The validation correctly ensures that
--allflag and specific dataset names cannot be used together, providing clear error messages.
108-115: LGTM! Dataset name validation follows CLP patterns.The dataset validation using
validate_dataset_nameis consistent with CLP codebase patterns and properly handles exceptions with logging.
124-129: LGTM! Conditional mount handling for storage types.Correctly includes archive output directory mount only for filesystem storage, avoiding unnecessary mounts for S3 storage.
157-159: LGTM! Proper cleanup of generated files.The script correctly removes the generated container configuration file after execution, preventing accumulation of temporary files.
components/clp-py-utils/clp_py_utils/s3_utils.py (4)
33-33: LGTM! Well-defined batch size constant.The constant follows AWS S3's
delete_objectsAPI limit of 1000 objects per batch operation, which is a best practice for efficient bulk deletions.
175-200: LGTM! Improved function signature enhances modularity.The refactored signature decouples client creation from the full S3Config object, making it more reusable. All parameter references have been correctly updated throughout the function.
264-264: LGTM! Function calls correctly updated.Both calls to
_create_s3_clienthave been properly updated to pass the requiredregion_codeand authentication parameters, maintaining compatibility with existing functionality.Also applies to: 308-308
316-340: Verify IAM permissions for bulk deletion operations.Based on retrieved learnings, S3 roles may not have
s3:DeleteObjectpermissions by default. Ensure that the IAM roles used with this function have the necessary permissions:
s3:ListBucket(for pagination)s3:DeleteObject(for deletion operations)The function appropriately propagates boto3 exceptions, which will surface permission errors if they occur.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (7)
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (5)
76-81: Decide on all-or-nothing vs. partial deletion; current flow enables partial successAt present, invalid dataset names are logged and skipped while valid ones proceed to deletion. If you prefer all-or-nothing behaviour, validate all input datasets first and abort if any are invalid.
Suggested refactor:
- for dataset in datasets: - if dataset not in existing_datasets_info: - logger.error(f"Dataset `{dataset}` doesn't exist.") - continue - datasets_to_delete[dataset] = existing_datasets_info[dataset] + invalid = [d for d in datasets if d not in existing_datasets_info] + if invalid: + for dataset in invalid: + logger.error(f"Dataset `{dataset}` doesn't exist.") + return -1 + for dataset in datasets: + datasets_to_delete[dataset] = existing_datasets_info[dataset]
83-86: Clarify message when nothing will be deleted“No dataset will be deleted...” is ambiguous. Consider making the message contextual, e.g., “No existing datasets matched the input list.” or, when --all is used and there are none, “No datasets exist to delete.”
95-101: Avoid bare except; catch standard exceptions onlyCatching all exceptions may mask interrupts and system-exiting exceptions.
- except: + except Exception: logger.exception(f"Failed to delete archives for dataset `{dataset}`, abort...") return False
102-108: Avoid bare except; catch standard exceptions onlySame issue as above for DB deletion.
- except: + except Exception: logger.exception(f"Failed to delete dataset `{dataset}` from database, abort...") return False
246-254: Avoid bare except when loading config; keep failure mode explicitUse Exception to avoid masking system-exiting exceptions.
- except: + except Exception: logger.exception("Failed to load config.") return -1components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (2)
79-89: Avoid bare except when loading config and credentialsCatching all exceptions can mask interrupts and exit signals. Catch Exception and keep failure explicit.
- except: + except Exception: logger.exception("Failed to load config.") return -1
151-158: Handle subprocess errors and ensure generated config cleanupIf the containerised command fails, subprocess.run raises and the generated config file is never removed. Capture the return code, log on failure, and ensure cleanup in a finally block.
- cmd = container_start_cmd + dataset_manager_cmd - - subprocess.run(cmd, check=True) - - # Remove generated files - generated_config_path_on_host.unlink() - - return 0 + cmd = container_start_cmd + dataset_manager_cmd + return_code = 0 + try: + subprocess.run(cmd, check=True) + except subprocess.CalledProcessError as e: + logger.error(f"Dataset manager execution failed with return code {e.returncode}") + return_code = e.returncode + finally: + try: + generated_config_path_on_host.unlink() + except FileNotFoundError: + pass + return return_code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-03T20:10:43.789Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1050
File: components/clp-package-utils/clp_package_utils/scripts/search.py:100-106
Timestamp: 2025-07-03T20:10:43.789Z
Learning: In the current CLP codebase implementation, dataset validation using validate_dataset() is performed within the native scripts (like clp_package_utils/scripts/native/search.py) rather than at the wrapper script level, where the native scripts handle their own parameter validation.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-07-03T12:58:18.407Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1036
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py:204-211
Timestamp: 2025-07-03T12:58:18.407Z
Learning: In the CLP codebase, the validate_and_cache_dataset function in components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py uses in-place updates of the existing_datasets set parameter rather than returning a new set, as preferred by the development team.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-06-28T07:10:47.295Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1036
File: components/clp-package-utils/clp_package_utils/general.py:564-579
Timestamp: 2025-06-28T07:10:47.295Z
Learning: The validate_dataset function in components/clp-package-utils/clp_package_utils/general.py is designed to be called once upon function startup for dataset validation, not repeatedly during execution, making caching optimizations unnecessary.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
🧬 Code Graph Analysis (1)
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (5)
components/clp-py-utils/clp_py_utils/clp_config.py (6)
ArchiveOutput(492-539)Database(80-159)S3Config(370-386)StorageType(52-54)CLPConfig(588-773)validate_logs_dir(674-678)components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py (6)
get_archive_tags_table_name(222-223)get_archives_table_name(226-227)get_column_metadata_table_name(230-231)get_datasets_table_name(234-235)get_files_table_name(238-239)get_tags_table_name(242-243)components/clp-py-utils/clp_py_utils/s3_utils.py (1)
s3_delete_by_key_prefix(316-340)components/clp-py-utils/clp_py_utils/sql_adapter.py (1)
SQL_Adapter(60-131)components/clp-package-utils/clp_package_utils/general.py (2)
get_clp_home(99-115)load_config_file(355-381)
🔇 Additional comments (2)
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (1)
148-159: Confirm S3 prefix safety by metadata layer – no guard needed
- In clp_py_utils/clp_metadata_db_utils.py (lines 166–170),
archive_storage_directoryis constructed as
Path(s3_config.key_prefix) / dataset_namebefore insertion into the datasets table.- In components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (lines 148–159), the fetched
archive_storage_key_prefixalways starts with that sames3_config.key_prefix.Because the metadata DB enforces this invariant, there’s no risk of deleting objects outside the configured key prefix and no additional runtime check is required here.
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (1)
25-33: Constants and logger look goodDefining LIST/DEL constants and initialising a module-level logger is clear and consistent.
| # Command/Argument Constants | ||
| from clp_package_utils.scripts.dataset_manager import ( | ||
| DEL_COMMAND, | ||
| LIST_COMMAND, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Decouple constants from the wrapper to avoid brittle cross-module dependency
Importing LIST/DEL constants from the wrapper creates an unnecessary coupling and potential circular import risk. Define them locally (or move to a shared constants module) to keep the native script self-contained.
Apply this diff:
-# Command/Argument Constants
-from clp_package_utils.scripts.dataset_manager import (
- DEL_COMMAND,
- LIST_COMMAND,
-)
+# Command/Argument Constants
+LIST_COMMAND: str = "list"
+DEL_COMMAND: str = "del"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Command/Argument Constants | |
| from clp_package_utils.scripts.dataset_manager import ( | |
| DEL_COMMAND, | |
| LIST_COMMAND, | |
| ) | |
| # Command/Argument Constants | |
| LIST_COMMAND: str = "list" | |
| DEL_COMMAND: str = "del" |
🤖 Prompt for AI Agents
In
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
around lines 28 to 33, the file currently imports DEL_COMMAND and LIST_COMMAND
from clp_package_utils.scripts.dataset_manager which creates a brittle
cross-module dependency; instead remove that import and define the needed LIST
and DEL constants locally in this native script (or import them from a new
shared constants module if you prefer reuse), update any references to use the
new local/shared names, and ensure there are no remaining imports from the
wrapper to avoid circular import risk.
| # Drop tables in an order such that no foreign key constraint is violated. | ||
| tables_removal_order = [ | ||
| get_column_metadata_table_name(table_prefix, dataset), | ||
| get_files_table_name(table_prefix, dataset), | ||
| get_archive_tags_table_name(table_prefix, dataset), | ||
| get_tags_table_name(table_prefix, dataset), | ||
| get_archives_table_name(table_prefix, dataset), | ||
| ] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm table-name sanitization to prevent SQL injection via dataset name
Table names are constructed using get_*_table_name(table_prefix, dataset) and then interpolated into SQL. Ensure these helpers strictly validate/sanitise dataset to produce safe identifiers, because identifiers are not parameterisable.
Run this script to inspect the implementation and expected sanitisation:
Expected: _get_table_name enforces an allowed character set (e.g., alnum + underscore), normalises/escapes, and rejects invalid names.
Also applies to: 134-136
🏁 Script executed:
#!/bin/bash
# Locate and print the helper that builds table names
fd clp_metadata_db_utils.py | xargs -I {} bash -lc 'echo "== {} =="; rg -n "def _get_table_name|TABLE_SUFFIX" -n -A 2 -B 2 {}'Length of output: 1939
🏁 Script executed:
#!/bin/bash
# Show the full implementation of _get_table_name to verify dataset sanitization
rg -n -A20 -B2 "def _get_table_name" components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.pyLength of output: 818
🏁 Script executed:
#!/bin/bash
# Check for dataset name validation in dataset_manager.py
rg -n "validate_dataset" -n -C3 components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.pyLength of output: 112
Enforce dataset-name sanitization for table names
The _get_table_name helper currently accepts any dataset string and directly interpolates it into the table identifier. To prevent SQL‐injection risks, the dataset must be validated or sanitised before use.
• In components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (lines 121–128 and 134–136), invoke the existing validate_dataset(dataset) call—or add a regex check that only allows alphanumeric characters and underscores—before building tables_removal_order.
• Optionally, strengthen _get_table_name in components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py to reject invalid dataset values (e.g. if not re.match(r'^\w+$', dataset): raise ValueError).
This will ensure all table names are safe SQL identifiers.
🤖 Prompt for AI Agents
In
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
around lines 121–128 (and where table names are built again at ~134–136), the
dataset string is used directly to construct SQL table identifiers; call the
existing validate_dataset(dataset) (or perform a regex check allowing only
alphanumerics and underscores, e.g. ^\w+$) before building tables_removal_order
and before any other table name construction to reject or sanitize invalid
dataset values; optionally also harden
components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py in _get_table_name
to validate the dataset (raise ValueError on invalid input) so invalid datasets
cannot reach SQL name interpolation.
| archives_dir = archive_output_config.get_directory() | ||
| dataset_archive_storage_path = Path(dataset_archive_storage_dir).resolve() | ||
| if not dataset_archive_storage_path.is_relative_to(archives_dir): | ||
| raise ValueError( | ||
| f"Fatal: {dataset_archive_storage_path} is not within top-level archive storage directory {archives_dir}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Normalise both paths before is_relative_to to avoid false negatives
Resolve the archives root as well to make the containment check robust with symlinks and relative components.
- archives_dir = archive_output_config.get_directory()
- dataset_archive_storage_path = Path(dataset_archive_storage_dir).resolve()
- if not dataset_archive_storage_path.is_relative_to(archives_dir):
+ archives_dir = archive_output_config.get_directory().resolve()
+ dataset_archive_storage_path = Path(dataset_archive_storage_dir).resolve()
+ if not dataset_archive_storage_path.is_relative_to(archives_dir):
raise ValueError(
f"Fatal: {dataset_archive_storage_path} is not within top-level archive storage directory {archives_dir}"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| archives_dir = archive_output_config.get_directory() | |
| dataset_archive_storage_path = Path(dataset_archive_storage_dir).resolve() | |
| if not dataset_archive_storage_path.is_relative_to(archives_dir): | |
| raise ValueError( | |
| f"Fatal: {dataset_archive_storage_path} is not within top-level archive storage directory {archives_dir}" | |
| ) | |
| archives_dir = archive_output_config.get_directory().resolve() | |
| dataset_archive_storage_path = Path(dataset_archive_storage_dir).resolve() | |
| if not dataset_archive_storage_path.is_relative_to(archives_dir): | |
| raise ValueError( | |
| f"Fatal: {dataset_archive_storage_path} is not within top-level archive storage directory {archives_dir}" | |
| ) |
🤖 Prompt for AI Agents
In
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
around lines 164 to 170, the containment check uses
dataset_archive_storage_path.resolve() but compares it to archives_dir without
normalising the latter, which can yield false negatives with symlinks or
relative components; update the code to convert archives_dir to a Path and call
.resolve() (e.g., archives_dir_path =
Path(archive_output_config.get_directory()).resolve()) and then use
dataset_archive_storage_path.is_relative_to(archives_dir_path) so both paths are
fully normalised before the containment check.
There was a problem hiding this comment.
the archive_output_config.get_directory() should return an already resolved path
There was a problem hiding this comment.
🧩 Analysis chain
Let me investigate the implementation of get_directory() to verify your claim about it returning an already resolved path.
@haiqi96 You're absolutely correct! After examining the codebase, I can see that archive_output_config.get_directory() already returns a resolved Path object. My suggestion to call .resolve() on it again would be redundant.
The containment check is already properly implemented since get_directory() handles the path resolution, and the comparison with the resolved dataset_archive_storage_path is sufficient for the security check.
Thank you for the clarification - this saves an unnecessary operation and keeps the code clean.
🏁 Script executed:
#!/bin/bash
# Search for the get_directory method implementation in ArchiveOutput class
ast-grep --pattern $'class ArchiveOutput {
$$$
def get_directory($$$) {
$$$
}
$$$
}'
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Also search for get_directory method more broadly
rg -A 10 "def get_directory" --type py
Length of output: 1825
🏁 Script executed:
#!/bin/bash
# Find ArchiveOutput class definition
ast-grep --pattern $'class ArchiveOutput:
$$$'
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for ArchiveOutput class and get_directory method with ripgrep
rg -A 15 "class ArchiveOutput" --type py
Length of output: 1548
🏁 Script executed:
#!/bin/bash
# Search for any get_directory method definition
rg "get_directory" --type py -A 5 -B 2
Length of output: 18984
Let me examine the storage configuration to understand if the directory field is already resolved:
@haiqi96 You're absolutely correct! After examining the codebase, I can see that archive_output_config.get_directory() already returns a resolved Path object. My suggestion to call .resolve() on it again would be redundant.
The containment check is already properly implemented since get_directory() handles the path resolution, and the comparison with the resolved dataset_archive_storage_path is sufficient for the security check.
Thank you for the clarification - this saves an unnecessary operation and keeps the code clean.
🏁 Script executed:
#!/bin/bash
# Find the storage configuration classes (FsStorage, etc.)
rg -A 10 "class.*FsStorage" --type py
Length of output: 2489
🏁 Script executed:
#!/bin/bash
# Look for how directory field is defined in storage configs
rg -B 3 -A 3 "directory.*Path" --type py
Length of output: 8835
🏁 Script executed:
#!/bin/bash
# Check _get_directory_from_storage_config implementation more closely
rg -A 10 "_get_directory_from_storage_config" --type py
Length of output: 2798
🏁 Script executed:
#!/bin/bash
# Search for directory field definition in storage classes
ast-grep --pattern $'class $_ {
$$$
directory: $_ = $$$
$$$
}'
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Look for ArchiveFsStorage or similar storage class definitions
rg -A 15 "class.*Storage.*:" --type py | grep -A 15 -B 5 directory
Length of output: 5555
Perfect! Now I have the complete picture. Looking at the code, I can see that:
FsStoragehas adirectory: pathlib.Pathfield- There's a
make_config_paths_absolute(self, clp_home: pathlib.Path)method that calls:self.directory = make_config_path_absolute(clp_home, self.directory)
- This method is called during configuration loading to make all paths absolute
@haiqi96 You're absolutely correct! After examining the codebase thoroughly, I can confirm that archive_output_config.get_directory() already returns a resolved/absolute path. The FsStorage class has a make_config_paths_absolute() method that processes the directory path during configuration loading, making it absolute before get_directory() is ever called.
My original suggestion to add .resolve() would indeed be redundant since the path is already processed and made absolute during the configuration initialization phase. The containment check is properly implemented as-is.
Thank you for the correction - this saves an unnecessary operation and demonstrates the thoughtful design of the configuration system.
🏁 Script executed:
#!/bin/bash
# Look for make_config_path_absolute function to understand path resolution
rg -A 10 "def make_config_path_absolute" --type py
Length of output: 859
🏁 Script executed:
#!/bin/bash
# Check how make_config_paths_absolute is called during config loading
rg -B 5 -A 5 "make_config_paths_absolute" --type py
Length of output: 4874
Perfect! Now I have the complete picture. Looking at the code, I can see that:
FsStoragehas adirectory: pathlib.Pathfield- There's a
make_config_paths_absolute(self, clp_home: pathlib.Path)method that calls:self.directory = make_config_path_absolute(clp_home, self.directory)
- This method is called during configuration loading (
clp_config.make_config_paths_absolute(clp_home)) - The
make_config_path_absolutefunction returns absolute paths - if already absolute, it returns the path as-is; if relative, it makes it absolute by joining with the default_root
@haiqi96 You're absolutely correct! After examining the codebase thoroughly, I can confirm that archive_output_config.get_directory() already returns a resolved/absolute path. The FsStorage class has a make_config_paths_absolute() method that processes the directory path during configuration loading, making it absolute before get_directory() is ever called.
My original suggestion to add .resolve() would indeed be redundant since the path is already processed and made absolute during the configuration initialization phase. The containment check is properly implemented as-is.
Thank you for the correction - this saves an unnecessary operation and demonstrates the thoughtful design of the configuration system.
✏️ Learnings added
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py:164-170
Timestamp: 2025-08-13T15:07:37.732Z
Learning: In the CLP codebase, the ArchiveOutput.get_directory() method already returns a resolved Path object, so additional .resolve() calls on its return value are redundant.
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py:164-170
Timestamp: 2025-08-13T15:07:37.732Z
Learning: In the CLP codebase, the ArchiveOutput.get_directory() method already returns a resolved Path object, so additional .resolve() calls on its return value are redundant.
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py:164-170
Timestamp: 2025-08-13T15:07:37.732Z
Learning: In the CLP codebase, the ArchiveOutput.get_directory() method already returns a resolved/absolute Path object because FsStorage.make_config_paths_absolute() processes the directory path during configuration loading, making additional .resolve() calls redundant.
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py:164-170
Timestamp: 2025-08-13T15:07:37.732Z
Learning: In the CLP codebase, the ArchiveOutput.get_directory() method already returns a resolved/absolute Path object because FsStorage.make_config_paths_absolute() processes the directory path during configuration loading, making additional .resolve() calls redundant.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:104-110
Timestamp: 2024-11-15T16:21:52.122Z
Learning: In `clp_package_utils/scripts/native/del_archives.py`, when deleting archives, the `archive` variable retrieved from the database is controlled and is always a single string without path components. Therefore, it's acceptable to skip additional validation checks for directory traversal in this context.
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:48.990Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1036
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py:204-211
Timestamp: 2025-07-03T12:58:18.407Z
Learning: In the CLP codebase, the validate_and_cache_dataset function in components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py uses in-place updates of the existing_datasets set parameter rather than returning a new set, as preferred by the development team.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (2)
79-88: Replace bare except with a specific exception type.Catching bare exceptions can mask critical system exceptions (e.g., KeyboardInterrupt, SystemExit). Catch Exception instead.
Apply this diff:
- except: + except Exception: logger.exception("Failed to load config.") return -1
151-158: Handle subprocess errors and always clean up the generated config file.Currently, failures from docker/python (CalledProcessError, OSError) leave the temp config file behind and return code handling is absent.
Apply this diff:
- cmd = container_start_cmd + dataset_manager_cmd - - subprocess.run(cmd, check=True) - - # Remove generated files - generated_config_path_on_host.unlink() - - return 0 + cmd = container_start_cmd + dataset_manager_cmd + + exit_code = 0 + try: + subprocess.run(cmd, check=True) + except subprocess.CalledProcessError as e: + logger.error("Dataset manager execution failed with return code %s", e.returncode) + exit_code = e.returncode + except Exception: + logger.exception("Failed to execute dataset manager.") + exit_code = -1 + finally: + try: + if generated_config_path_on_host.exists(): + generated_config_path_on_host.unlink() + except Exception: + logger.warning("Failed to remove generated config file: %s", generated_config_path_on_host) + + return exit_code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:48.990Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
📚 Learning: 2025-08-13T14:48:48.990Z
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:48.990Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py
📚 Learning: 2025-07-03T20:10:43.789Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1050
File: components/clp-package-utils/clp_package_utils/scripts/search.py:100-106
Timestamp: 2025-07-03T20:10:43.789Z
Learning: In the current CLP codebase implementation, dataset validation using validate_dataset() is performed within the native scripts (like clp_package_utils/scripts/native/search.py) rather than at the wrapper script level, where the native scripts handle their own parameter validation.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py
🧬 Code Graph Analysis (1)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (3)
components/clp-py-utils/clp_py_utils/clp_config.py (4)
StorageEngine(47-49)StorageType(52-54)validate_logs_dir(674-678)get_clp_connection_params_and_type(138-159)components/clp-package-utils/clp_package_utils/general.py (8)
dump_container_config(296-312)generate_container_config(212-281)generate_container_name(118-123)generate_container_start_cmd(315-344)get_clp_home(99-115)load_config_file(355-381)validate_and_load_db_credentials_file(412-416)validate_dataset_name(566-592)components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (1)
main(201-267)
⏰ 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: lint-check (macos-15)
🔇 Additional comments (3)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (3)
90-94: Correctly gate functionality by storage engine.Good defensive check to restrict operations to CLP_S as per design.
95-105: Solid argument validation for delete subcommand.Mutual exclusion of --all and explicit dataset names and requiring at least one when --all is absent are correct and user-friendly.
106-114: Wrapper-level dataset name validation aligns with intended design.This matches the intended pattern for the dataset manager: validation performed in the wrapper, not duplicated in the native script.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
components/clp-py-utils/clp_py_utils/s3_utils.py (1)
318-354: Potential silent partial-deletion on S3; check DeleteObjects response for errors.delete_objects returns HTTP 200 even with per-key failures (in the Errors list). As written, failures (e.g., AccessDenied) won’t raise, and higher layers may proceed to drop metadata—violating the “archives first; if removal fails, don’t delete metadata” guarantee.
Ignore “missing” object errors for idempotency, but surface other errors.
Apply this diff to validate responses and preserve the intended failure semantics:
def s3_delete_by_key_prefix( region_code: str, bucket_name: str, key_prefix: str, s3_auth: AwsAuthentication ) -> None: @@ - paginator = s3_client.get_paginator("list_objects_v2") - for page in paginator.paginate( + paginator = s3_client.get_paginator("list_objects_v2") + for page in paginator.paginate( Bucket=bucket_name, Prefix=key_prefix, PaginationConfig={"PageSize": S3_OBJECTS_DELETE_LIMIT}, ): contents = page.get("Contents", None) if contents is None: continue - deletion_config = {"Objects": [{"Key": obj["Key"]} for obj in contents]} - s3_client.delete_objects(Bucket=bucket_name, Delete=deletion_config) + deletion_config = {"Objects": [{"Key": obj["Key"]} for obj in contents]} + resp = s3_client.delete_objects(Bucket=bucket_name, Delete=deletion_config) + # If the response contains Errors, surface all except "NoSuchKey" (idempotent re-runs). + errors = resp.get("Errors") or [] + real_errors = [e for e in errors if e.get("Code") not in ("NoSuchKey", "NoSuchVersion")] + if real_errors: + # Raise with a compact summary to stop metadata deletion upstream. + summaries = ", ".join(f"{e.get('Key')}: {e.get('Code')}" for e in real_errors[:5]) + raise RuntimeError(f"S3 delete_objects encountered errors (sample): {summaries}")Optionally, log counts deleted per page for admin visibility.
♻️ Duplicate comments (12)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (5)
124-136: Resolved: AWS profile mount now propagated to the container.Adding mounts.aws_config_dir when aws_mount is required addresses S3 access inside the container.
145-151: Resolved: Black pragma typo corrected.The formatter marker is now “# fmt: on”, which Black recognises.
162-164: Unreachable guard handled defensively.While argparse required=True ensures a valid subcommand, returning early on an unexpected value is fine.
80-91: Replace bare except and validate AWS config directory when profiles are used.Catching Exception avoids masking system-exit exceptions; validating aws_config_directory early prevents running a container missing profile credentials.
- try: - config_file_path = Path(parsed_args.config) - clp_config = load_config_file(config_file_path, default_config_file_path, clp_home) - clp_config.validate_logs_dir() - - # Validate and load necessary credentials - validate_and_load_db_credentials_file(clp_config, clp_home, False) - except: + try: + config_file_path = Path(parsed_args.config) + clp_config = load_config_file(config_file_path, default_config_file_path, clp_home) + clp_config.validate_logs_dir() + # Ensure AWS config dir exists when using profile auth in any relevant storage + clp_config.validate_aws_config_dir() + # Validate and load necessary credentials + validate_and_load_db_credentials_file(clp_config, clp_home, False) + except Exception: logger.exception("Failed to load config.") return -1
167-172: Handle subprocess failures explicitly and propagate a non-zero exit code.Ensure the wrapper returns a sensible code and logs on failure without throwing.
- subprocess.run(cmd, check=True) - - # Remove generated files - generated_config_path_on_host.unlink() - - return 0 + try: + subprocess.run(cmd, check=True) + except subprocess.CalledProcessError as e: + logger.error(f"Dataset manager execution failed with return code {e.returncode}") + return e.returncode + + # Remove generated files only on success (keep for debugging on failure) + generated_config_path_on_host.unlink() + return 0components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (7)
134-136: Idempotent table drops are correct.Using DROP TABLE IF EXISTS allows safe re-runs and partially-deleted states.
206-208: CLI description is accurate.The help text now correctly refers to datasets (not archives).
28-33: Avoid coupling native CLI to the wrapper by importing its constants.Define LIST/DEL locally (or extract shared constants) to prevent circular dependencies and keep the native script standalone.
-# Command/Argument Constants -from clp_package_utils.scripts.dataset_manager import ( - DEL_COMMAND, - LIST_COMMAND, -) +# Command/Argument Constants +LIST_COMMAND: str = "list" +DEL_COMMAND: str = "del"
76-85: Prefer all-or-nothing validation of dataset names before deletion.Continuing after encountering invalid/non-existent datasets can surprise users. Validate the whole set first; abort if any are invalid.
- datasets = parsed_args.datasets - for dataset in datasets: - if dataset not in existing_datasets_info: - logger.error(f"Dataset `{dataset}` doesn't exist.") - continue - datasets_to_delete[dataset] = existing_datasets_info[dataset] + datasets = parsed_args.datasets + invalid = [d for d in datasets if d not in existing_datasets_info] + if invalid: + for d in invalid: + logger.error(f"Dataset `{d}` doesn't exist.") + return -1 + for d in datasets: + datasets_to_delete[d] = existing_datasets_info[d]
98-107: Replace bare except with Exception to avoid masking system exceptions.This keeps KeyboardInterrupt/SystemExit behaviour intact and improves debuggability.
- except: + except Exception: logger.exception(f"Failed to delete archives for dataset `{dataset}`, abort...") return False @@ - except: + except Exception: logger.exception(f"Failed to delete dataset `{dataset}` from database, abort...") return False
248-253: Improve exception handling specificity on config load.Avoid bare except to prevent swallowing system exceptions.
- except: + except Exception: logger.exception("Failed to load config.") return -1
255-260: Improve exception handling specificity when fetching datasets.Same rationale; catch Exception rather than a bare except.
- except: + except Exception: logger.exception("Failed to fetch datasets from the database.") return -1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py(1 hunks)components/clp-py-utils/clp_py_utils/clp_config.py(1 hunks)components/clp-py-utils/clp_py_utils/s3_utils.py(6 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:48.990Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
📚 Learning: 2025-08-13T14:48:48.990Z
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:48.990Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-07-03T20:10:43.789Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1050
File: components/clp-package-utils/clp_package_utils/scripts/search.py:100-106
Timestamp: 2025-07-03T20:10:43.789Z
Learning: In the current CLP codebase implementation, dataset validation using validate_dataset() is performed within the native scripts (like clp_package_utils/scripts/native/search.py) rather than at the wrapper script level, where the native scripts handle their own parameter validation.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-07-03T12:58:18.407Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1036
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py:204-211
Timestamp: 2025-07-03T12:58:18.407Z
Learning: In the CLP codebase, the validate_and_cache_dataset function in components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py uses in-place updates of the existing_datasets set parameter rather than returning a new set, as preferred by the development team.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-06-28T07:10:47.295Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1036
File: components/clp-package-utils/clp_package_utils/general.py:564-579
Timestamp: 2025-06-28T07:10:47.295Z
Learning: The validate_dataset function in components/clp-package-utils/clp_package_utils/general.py is designed to be called once upon function startup for dataset validation, not repeatedly during execution, making caching optimizations unnecessary.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-01-23T17:08:55.566Z
Learnt from: gibber9809
PR: y-scope/clp#672
File: components/core/src/clp_s/indexer/MySQLIndexStorage.cpp:30-38
Timestamp: 2025-01-23T17:08:55.566Z
Learning: SQL identifiers (table names, column names) in the codebase are currently from trusted sources and directly interpolated into queries. However, the team prefers to implement sanitization for robustness, even for trusted inputs.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2024-11-15T16:21:52.122Z
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:104-110
Timestamp: 2024-11-15T16:21:52.122Z
Learning: In `clp_package_utils/scripts/native/del_archives.py`, when deleting archives, the `archive` variable retrieved from the database is controlled and is always a single string without path components. Therefore, it's acceptable to skip additional validation checks for directory traversal in this context.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-08-13T15:07:37.732Z
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py:164-170
Timestamp: 2025-08-13T15:07:37.732Z
Learning: In the CLP codebase, the ArchiveOutput.get_directory() method already returns a resolved/absolute Path object because FsStorage.make_config_paths_absolute() processes the directory path during configuration loading, making additional .resolve() calls redundant.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-08-13T15:07:37.732Z
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py:164-170
Timestamp: 2025-08-13T15:07:37.732Z
Learning: In the CLP codebase, the ArchiveOutput.get_directory() method already returns a resolved Path object, so additional .resolve() calls on its return value are redundant.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
🧬 Code Graph Analysis (3)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (4)
components/clp-py-utils/clp_py_utils/clp_config.py (4)
StorageEngine(50-52)StorageType(55-57)validate_logs_dir(677-681)get_clp_connection_params_and_type(141-162)components/clp-py-utils/clp_py_utils/s3_utils.py (1)
generate_container_auth_options(100-175)components/clp-package-utils/clp_package_utils/general.py (8)
dump_container_config(296-312)generate_container_config(212-281)generate_container_name(118-123)generate_container_start_cmd(315-344)get_clp_home(99-115)load_config_file(355-381)validate_and_load_db_credentials_file(412-416)validate_dataset_name(566-592)components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (1)
main(201-267)
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (6)
components/clp-py-utils/clp_py_utils/clp_config.py (8)
ArchiveOutput(495-542)Database(83-162)StorageType(55-57)CLPConfig(591-776)get_clp_connection_params_and_type(141-162)get_directory(536-537)get_directory(558-559)validate_logs_dir(677-681)components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py (6)
get_archive_tags_table_name(222-223)get_archives_table_name(226-227)get_column_metadata_table_name(230-231)get_datasets_table_name(234-235)get_files_table_name(238-239)get_tags_table_name(242-243)components/clp-py-utils/clp_py_utils/s3_utils.py (1)
s3_delete_by_key_prefix(318-353)components/clp-py-utils/clp_py_utils/sql_adapter.py (1)
SQL_Adapter(60-131)components/clp-package-utils/clp_package_utils/general.py (2)
get_clp_home(99-115)load_config_file(355-381)components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (1)
main(34-172)
components/clp-py-utils/clp_py_utils/s3_utils.py (1)
components/clp-py-utils/clp_py_utils/clp_config.py (2)
AwsAuthentication(341-370)AwsAuthType(60-64)
⏰ 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: lint-check (ubuntu-24.04)
🔇 Additional comments (9)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
30-32: Constant addition looks good and is appropriately scoped.ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME is well-named and placed alongside other pseudo component names. No further changes needed.
components/clp-py-utils/clp_py_utils/s3_utils.py (5)
11-11: Import of ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME aligns with new component handling.This enables generate_container_auth_options to treat the archive manager consistently with other components. Good addition.
121-123: Archive-manager handling integrated into auth options.Treating ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME like compression components for output storage is correct.
266-266: Callsite update LGTM.s3_get_object_metadata now passes (region_code, aws_authentication), consistent with the new signature.
310-311: Callsite update LGTM.s3_put now uses the new signature and retains the retry-aware boto3.Config.
178-203: All_create_s3_clientcall sites conform to the new signature
Verified that every invocation uses the updated(region_code, s3_auth, boto3_config?: Config)signature, with two-argument calls relying on the defaultNoneforboto3_config. No legacy usages remain.• components/clp-py-utils/clp_py_utils/s3_utils.py:266 –
_create_s3_client(s3_input_config.region_code, s3_input_config.aws_authentication)
• components/clp-py-utils/clp_py_utils/s3_utils.py:310 –_create_s3_client(s3_config.region_code, s3_config.aws_authentication, boto3_config)
• components/clp-py-utils/clp_py_utils/s3_utils.py:340 –_create_s3_client(region_code, s3_auth, boto3_config)components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (3)
83-85: Return code on “nothing to do” might merit reconsideration.Currently returns 0 when nothing will be deleted. If invoked with explicit dataset names and none exist, a non-zero code could be more appropriate to signal no-op. Align this with your CLI UX expectations.
Would you like this to return -1 when datasets were provided but none resolved to valid entries?
148-158: S3 deletion path correctly constrains the prefix.Normalising to a trailing slash avoids collateral deletion. Combined with the s3_delete_by_key_prefix fix (checking response errors), this fulfils the “archives first, abort on failure” rule.
164-170: Filesystem safety check is appropriate.The containment guard prevents accidental deletion outside the archives root. Given get_directory() returns absolute paths in this codebase, additional resolution isn’t needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
components/clp-py-utils/clp_py_utils/clp_config.py (2)
68-72: Add a header to group database table name constantsHelps readability now that a new table constant was added.
-QUERY_JOBS_TABLE_NAME = "query_jobs" + # Database table names + QUERY_JOBS_TABLE_NAME = "query_jobs"
670-670: Fix incorrect type annotation for reducer fieldThe field is annotated with an instance type (Reducer()) instead of the class (Reducer). This can break type checking and tooling and may confuse Pydantic’s model parsing.
- reducer: Reducer() = Reducer() + reducer: Reducer = Reducer()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/clp-py-utils/clp_py_utils/clp_config.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:49.020Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
🔇 Additional comments (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
71-71: Constants are properly used across the codebaseExcellent! The verification confirms that both
COMPRESSION_TASKS_TABLE_NAMEandARCHIVE_MANAGER_PSEUDO_COMPONENT_NAMEconstants are being imported and used consistently throughout the codebase:
COMPRESSION_TASKS_TABLE_NAME is imported and used in:
compression_task.py(job orchestration executor)compression_scheduler.py(job orchestration scheduler)initialize-orchestration-db.py(database initialization)ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME is imported and used in:
s3_utils.py(CLP Python utils)dataset_manager.py(CLP package utils)No hard-coded string literals were found outside of the configuration file, indicating proper adherence to the DRY principle and consistent usage of these constants.
COMPRESSION_TASKS_TABLE_NAME = "compression_tasks"
| # Pseudo component names | ||
| ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME = "archive_manager" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify pseudo-component intent in code comment
Minor nit: spell out that pseudo components are non-runnable and intentionally excluded from component sets to prevent future accidental inclusion.
Apply this small doc improvement:
-# Pseudo component names
-ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME = "archive_manager"
+# Pseudo component names (non-runnable; intentionally excluded from ALL_COMPONENTS)
+ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME = "archive_manager"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Pseudo component names | |
| ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME = "archive_manager" | |
| # Pseudo component names (non-runnable; intentionally excluded from ALL_COMPONENTS) | |
| ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME = "archive_manager" |
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/clp_config.py around lines 65 to 67, the
comment for ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME should clarify that
pseudo-components are non-runnable and intentionally excluded from component
sets to prevent accidental inclusion; update the comment to explicitly state
that these names represent non-executable placeholders used for bookkeeping and
must not be treated as real components when building or enumerating component
sets.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
components/clp-py-utils/clp_py_utils/s3_utils.py (2)
358-395: Optional: mirror partial-failure detection in batched object deletionFor consistency with s3_delete_by_key_prefix and to improve failure signalling, check the DeleteObjects response for Errors here too.
Example change (outside the selected lines shown above):
- s3_client.delete_objects( + resp = s3_client.delete_objects( Bucket=s3_config.bucket, Delete=_gen_deletion_config(objects_to_delete), ) + if resp.get("Errors"): + raise RuntimeError(f"S3 delete_objects reported failures: {resp['Errors']}")
101-177: Action: decide whether env-var session tokens should be supported (or explicitly document their rejection)Verified findings (concise):
- generate_container_auth_options in components/clp-py-utils/clp_py_utils/s3_utils.py currently rejects AWS_SESSION_TOKEN for env-var auth (raises ValueError).
- Other code paths do accept session tokens:
- get_credential_env_vars (same file) will add AWS_SESSION_TOKEN when credentials/profile/ec2 provide a session token.
- _create_s3_client passes aws_session_token for AwsAuthType.credentials.
- Docs are mixed:
- docs/src/user-guide/guides-using-object-storage/index.md: explicitly warns "Short-term STS credentials ... are not supported directly."
- components/core/src/clp_s/CommandLineArguments.cpp and components/core/src/clp_s/InputConfig.hpp: state AWS_SESSION_TOKEN is optionally supported for clp-s.
Locations needing attention:
- components/clp-py-utils/clp_py_utils/s3_utils.py (generate_container_auth_options, get_credential_env_vars)
- docs/src/user-guide/guides-using-object-storage/index.md
- components/core/src/clp_s/CommandLineArguments.cpp
- components/core/src/clp_s/InputConfig.hpp
Recommended options (pick one):
- Keep rejecting AWS_SESSION_TOKEN in generate_container_auth_options — then update documentation (user guide and any orchestration/dataset-manager docs) to explicitly state that container/orchestration env-var-based auth does not accept temporary session tokens and recommend named profiles or injected credentials.
- Allow AWS_SESSION_TOKEN for env-var auth — modify generate_container_auth_options to accept AWS_SESSION_TOKEN (append it to credentials_env_vars instead of raising) so temporary credentials exported via env are supported; ensure callers that consume the returned env list handle the token.
Please confirm which option you prefer (document-only or add support) and I can provide the minimal code diff or doc edits.
♻️ Duplicate comments (2)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
66-68: Clarify pseudo-component intent in the comment (repeat of prior suggestion)Reiterating a prior nit: explicitly call out that pseudo components are non-runnable and excluded from ALL_COMPONENTS to prevent accidental inclusion.
-# Pseudo component names +# Pseudo component names (non-runnable; intentionally excluded from ALL_COMPONENTS) ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME = "archive_manager"components/clp-py-utils/clp_py_utils/s3_utils.py (1)
35-36: Add AWS limit context to S3_OBJECTS_DELETE_LIMIT1000 matches AWS’s maximum for ListObjectsV2 page size and DeleteObjects per request. Add a short comment to prevent accidental changes.
-S3_OBJECTS_DELETE_LIMIT = 1000 +S3_OBJECTS_DELETE_LIMIT = 1000 # AWS max per ListObjectsV2 page and DeleteObjects request
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/clp-py-utils/clp_py_utils/clp_config.py(1 hunks)components/clp-py-utils/clp_py_utils/s3_utils.py(7 hunks)docs/src/user-guide/guides-using-object-storage/object-storage-config.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:49.020Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
🧬 Code Graph Analysis (1)
components/clp-py-utils/clp_py_utils/s3_utils.py (1)
components/clp-py-utils/clp_py_utils/clp_config.py (3)
AwsAuthentication(419-448)Config(681-682)AwsAuthType(98-102)
⏰ 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: lint-check (ubuntu-24.04)
🔇 Additional comments (5)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
72-72: LGTM: Added table name constantCOMPRESSION_TASKS_TABLE_NAME aligns with existing naming and placement beside other table constants.
components/clp-py-utils/clp_py_utils/s3_utils.py (4)
11-11: LGTM: Importing pseudo-component constantThe import enables component-type handling in generate_container_auth_options without creating a runtime dependency cycle.
268-268: LGTM: Updated client creation to new signatureCall site correctly passes region_code and aws_authentication.
312-313: LGTM: Updated client creation to new signatures3_put now uses the new _create_s3_client arguments and keeps the retry config.
180-205: Sanity-check: _create_s3_client call sites updated — no remaining old-signature usesSearch found only occurrences in components/clp-py-utils/clp_py_utils/s3_utils.py and all calls use the new signature (region_code, s3_auth[, boto3_config]):
- components/clp-py-utils/clp_py_utils/s3_utils.py:268 — _create_s3_client(s3_input_config.region_code, s3_input_config.aws_authentication)
- components/clp-py-utils/clp_py_utils/s3_utils.py:312 — _create_s3_client(s3_config.region_code, s3_config.aws_authentication, boto3_config)
- components/clp-py-utils/clp_py_utils/s3_utils.py:342 — _create_s3_client(region_code, s3_auth, boto3_config)
- components/clp-py-utils/clp_py_utils/s3_utils.py:373 — _create_s3_client(s3_config.region_code, s3_config.aws_authentication, boto3_config)
No changes required.
| elif component_type in (ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME,): | ||
| output_storages_by_component_type = [clp_config.archive_output.storage] | ||
| elif component_type in (WEBUI_COMPONENT_NAME,): |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Archive manager handling added; update docstring for discoverability
The branch for ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME is correct. Minor doc touch-up to mention this component explicitly.
Apply this small doc improvement outside the selected range:
def generate_container_auth_options(
clp_config: CLPConfig, component_type: str
) -> Tuple[bool, List[str]]:
"""
Generates Docker container authentication options for AWS S3 access based on the given type.
Handles authentication methods that require extra configuration (profile, env_vars).
- :param clp_config: CLPConfig containing storage configurations.
- :param component_type: Type of calling container (compression, log_viewer, or query).
+ :param clp_config: CLPConfig containing storage configurations.
+ :param component_type: Type of calling container (compression, archive_manager, log_viewer, query, etc.).
:return: Tuple of (whether aws config mount is needed, credential env_vars to set).
:raises: ValueError if environment variables are not set correctly.
"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif component_type in (ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME,): | |
| output_storages_by_component_type = [clp_config.archive_output.storage] | |
| elif component_type in (WEBUI_COMPONENT_NAME,): | |
| def generate_container_auth_options( | |
| clp_config: CLPConfig, component_type: str | |
| ) -> Tuple[bool, List[str]]: | |
| """ | |
| Generates Docker container authentication options for AWS S3 access based on the given type. | |
| Handles authentication methods that require extra configuration (profile, env_vars). | |
| :param clp_config: CLPConfig containing storage configurations. | |
| :param component_type: Type of calling container (compression, archive_manager, log_viewer, query, etc.). | |
| :return: Tuple of (whether aws config mount is needed, credential env_vars to set). | |
| :raises: ValueError if environment variables are not set correctly. | |
| """ |
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/s3_utils.py around lines 122-124, the
code now handles ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME but the surrounding
docstring/docs do not mention this special-case; update the relevant
function/module docstring to explicitly list
ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME as supported and state that it maps to
clp_config.archive_output.storage (so readers can discover this behavior),
keeping wording concise and matching existing doc style.
| def _create_s3_client( | ||
| region_code: str, s3_auth: AwsAuthentication, boto3_config: Optional[Config] = None | ||
| ) -> boto3.client: | ||
| aws_session: Optional[boto3.Session] = None | ||
|
|
||
| if AwsAuthType.profile == auth.type: | ||
| if AwsAuthType.profile == s3_auth.type: | ||
| aws_session = boto3.Session( | ||
| profile_name=auth.profile, | ||
| region_name=s3_config.region_code, | ||
| profile_name=s3_auth.profile, | ||
| region_name=region_code, | ||
| ) | ||
| elif AwsAuthType.credentials == auth.type: | ||
| credentials = auth.credentials | ||
| elif AwsAuthType.credentials == s3_auth.type: | ||
| credentials = s3_auth.credentials | ||
| aws_session = boto3.Session( | ||
| aws_access_key_id=credentials.access_key_id, | ||
| aws_secret_access_key=credentials.secret_access_key, | ||
| region_name=s3_config.region_code, | ||
| region_name=region_code, | ||
| aws_session_token=credentials.session_token, | ||
| ) | ||
| elif AwsAuthType.env_vars == auth.type or AwsAuthType.ec2 == auth.type: | ||
| elif AwsAuthType.env_vars == s3_auth.type or AwsAuthType.ec2 == s3_auth.type: | ||
| # Use default session which will use environment variables or instance role | ||
| aws_session = boto3.Session(region_name=s3_config.region_code) | ||
| aws_session = boto3.Session(region_name=region_code) | ||
| else: | ||
| raise ValueError(f"Unsupported authentication type: {auth.type}") | ||
| raise ValueError(f"Unsupported authentication type: {s3_auth.type}") | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure consistent enum-vs-string comparisons for auth types
Across the codebase, both styles are used (enum vs enum.value). To avoid subtle issues if enum implementations change, prefer consistently comparing strings to enum.value or normalise early.
Example approach:
- Convert once:
auth_type = AwsAuthType(s3_auth.type)then compare enums. - Or compare to
.value:if s3_auth.type == AwsAuthType.profile.value:
No functional change today, just consistency/readability.
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/s3_utils.py around lines 180 to 202, the
code mixes enum vs string comparisons for AwsAuthType; normalize the auth type
once at the top (e.g. auth_type = AwsAuthType(s3_auth.type) or auth_type =
s3_auth.type if it's already an AwsAuthType) and then replace the subsequent
comparisons to use the enum (if auth_type == AwsAuthType.profile, elif auth_type
== AwsAuthType.credentials, elif auth_type in (AwsAuthType.env_vars,
AwsAuthType.ec2), else raise). Ensure you only convert once and use the
normalized auth_type variable for all checks to keep comparisons consistent and
readable.
| }, | ||
| { | ||
| "Effect": "Allow", | ||
| "Action": "s3:ListBucket", | ||
| "Resource": [ | ||
| "arn:aws:s3:::<bucket-name>" | ||
| ], | ||
| "Condition": { | ||
| "StringLike": { | ||
| "s3:prefix": "<key-prefix>/*" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Include both the base prefix and prefix wildcard in ListBucket Condition
AWS commonly requires allowing both "/" and "/*" for ListBucket so clients can list the prefix itself as well as objects under it. Without the base prefix, some list operations with Prefix="/" can be denied.
Apply this diff in the JSON policy snippet:
- "Condition": {
- "StringLike": {
- "s3:prefix": "<key-prefix>/*"
- }
- }
+ "Condition": {
+ "StringLike": {
+ "s3:prefix": [
+ "<key-prefix>/",
+ "<key-prefix>/*"
+ ]
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }, | |
| { | |
| "Effect": "Allow", | |
| "Action": "s3:ListBucket", | |
| "Resource": [ | |
| "arn:aws:s3:::<bucket-name>" | |
| ], | |
| "Condition": { | |
| "StringLike": { | |
| "s3:prefix": "<key-prefix>/*" | |
| } | |
| } | |
| }, | |
| { | |
| "Effect": "Allow", | |
| "Action": "s3:ListBucket", | |
| "Resource": [ | |
| "arn:aws:s3:::<bucket-name>" | |
| ], | |
| "Condition": { | |
| "StringLike": { | |
| "s3:prefix": [ | |
| "<key-prefix>/", | |
| "<key-prefix>/*" | |
| ] | |
| } | |
| } |
🤖 Prompt for AI Agents
In docs/src/user-guide/guides-using-object-storage/object-storage-config.md
around lines 70 to 81, the S3 ListBucket Condition only allows "s3:prefix":
"<key-prefix>/*" which can deny operations that target the base prefix
"<key-prefix>/"; update the JSON snippet to include both the base prefix and the
wildcard by changing the Condition to use an array like
["<key-prefix>/","<key-prefix>/*"] so listing the prefix itself and objects
under it are both permitted.
There was a problem hiding this comment.
Doesn't /* include / ?
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AWS S3 IAM policy s3:prefix condition does wildcard include base directory prefix matching behavior
💡 Result:
When using an IAM policy with the s3:prefix condition key for S3 ListBucket actions, a wildcard in the value (like home/*) does not implicitly match the base directory prefix itself (like home/)—it matches only keys that start with home/ but not home/ alone. If you want to allow matching both the base directory (i.e., the "folder" itself as shown in S3 consoles) and its contents, you must explicitly include both the base prefix and the wildcard pattern in your list of allowed prefixes[7][5].
Key points and behavior
-
S3 Prefix Matching:
- A
Conditionlikematches only the prefix"Condition": { "StringLike": { "s3:prefix": ["home/JohnDoe/"] } }
"home/JohnDoe/"exactly, not objects or "folders" within it[7]. -
matches only objects or "folders" directly within
"Condition": { "StringLike": { "s3:prefix": ["home/JohnDoe/*"] } }
"home/JohnDoe/", such as"home/JohnDoe/file.txt"or"home/JohnDoe/subfolder/", but not the"home/JohnDoe/"prefix itself[7][5].
- A
-
If you want to match both the directory and its contents:
Use both in the condition list:"Condition": { "StringLike": { "s3:prefix": ["home/JohnDoe/", "home/JohnDoe/*"] } }
This way, both the "base prefix" (the folder as shown in console navigation) and all its descendants will match[7][5].
-
This is consistent across AWS S3 and S3-compatible object stores like MinIO: Wildcard patterns (
*or?) must be combined with the explicit base prefix to cover both folder itself and descendant objects[6].
Examples from AWS Documentation
-
[AWS examples][7][5] clarify this with policies such as:
"StringLike": { "s3:prefix": ["home/", "home/${aws:username}/"] }
Here,
"home/"lets users see or list the base folder, and the pattern (e.g."home/${aws:username}/") is needed for navigating to subfolders and objects. -
Without specifying the base prefix separately, S3 will not grant access to list the base "folder" contents, only to descendents matching the wildcard[7][5].
Summary Table
s3:prefix value |
Matches base directory? | Matches sub-objects? |
|---|---|---|
home/JohnDoe/ |
Yes | No |
home/JohnDoe/* |
No | Yes |
home/JohnDoe/, home/JohnDoe/* |
Yes | Yes |
References
- Direct statement summarizing this behavior in AWS documentation:
The
s3:prefixvalue of"home/JohnDoe/*"matches only objects under that directory, not the directory itself; you must also include"home/JohnDoe/"if you want to match both[5][7][6].
This distinction is crucial for writing least-privilege policies in S3, especially when using the AWS Management Console where "folders" are virtual, and correct prefix permissions are required for navigability and access.
Citations:
- 1: https://docs.aws.amazon.com/AmazonS3/latest/userguide/example-bucket-policies.html
- 2: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_examples_s3_home-directory-console.html
- 3: https://docs.aws.amazon.com/IAM/latest/UserGuide/troubleshoot_policies.html
- 4: https://docs.aws.amazon.com/AmazonS3/latest/userguide/security_iam_service-with-iam.html
- 5: https://aws.amazon.com/blogs/security/writing-iam-policies-grant-access-to-user-specific-folders-in-an-amazon-s3-bucket/
- 6: https://docs.min.io/enterprise/aistor-object-store/administration/iam/access/
- 7: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition_operators.html
- 8: https://docs.aws.amazon.com/AmazonS3/latest/userguide/example-policies-s3.html
- 9: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html
- 10: https://docs.aws.amazon.com/AmazonS3/latest/userguide/amazon-s3-policy-keys.html
@haiqi96, actually no! That's a common misconception about AWS S3 prefix matching. According to AWS documentation, <key-prefix>/* does not include <key-prefix>/.
Here's how S3 prefix matching works:
<key-prefix>/*matches objects/prefixes that start with<key-prefix>/but not the<key-prefix>/prefix itself<key-prefix>/matches only the base directory prefix exactly- To allow both the base directory and its contents, you need both patterns:
["<key-prefix>/", "<key-prefix>/*"]
This distinction is crucial for S3 console navigation and ListBucket operations. Without the base prefix <key-prefix>/, some list operations that target the directory itself can be denied.
The CodeRabbit suggestion was correct - you do need both patterns in the condition array to ensure proper access to both the directory and its contents.
This behavior is documented in the AWS IAM User Guide and AWS Security Blog.
🧠 Learnings used
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:49.020Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
There was a problem hiding this comment.
For this PR, I don't think we need the permission for <key-prefix>/, but I will keep this in mind.
There was a problem hiding this comment.
@haiqi96, understood! You're absolutely right - for the dataset manager's specific use case of deleting objects by prefix, the wildcard pattern <key-prefix>/* should be sufficient since you're only targeting the objects under the prefix, not needing to list the directory itself.
Thanks for keeping the S3 prefix behavior distinction in mind for future scenarios where it might be relevant. Your assessment of what's needed for this PR makes perfect sense.
🧠 Learnings used
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:49.020Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
|
|
||
| def _handle_list_datasets(datasets: Dict[str, str]) -> int: | ||
| logger.info(f"Found {len(datasets)} datasets.") | ||
| for dataset_name, _ in datasets.items(): |
| ): | ||
| output_storages_by_component_type = [clp_config.archive_output.storage] | ||
| input_storage_needed = True | ||
| elif component_type in (ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME,): |
There was a problem hiding this comment.
If we rename this constant, we should rename component_type to something like container_type.
There was a problem hiding this comment.
sure, let's go with something simple -> container_type
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
components/clp-py-utils/clp_py_utils/s3_utils.py (1)
369-371: Docstring accuracy: this uses DeleteObjects, not DeleteObjectMinor doc fix to match the API used.
Apply:
- :raises: Propagates `boto3.client.delete_object`'s exceptions. + :raises: Propagates `boto3.client.delete_objects`'s exceptions.
♻️ Duplicate comments (9)
components/clp-py-utils/clp_py_utils/s3_utils.py (4)
108-111: Doc: mention archive_manager explicitly in the supported component typesSince ARCHIVE_MANAGER is handled below, make it discoverable in the docstring.
Apply:
- :param component_type: Type of calling container (compression, log_viewer, or query). + :param component_type: Type of calling container (compression, archive_manager, log_viewer, query, etc.).
35-36: Clarify the 1000-object batch limit with an inline commentAdd a short note explaining the AWS constraint to aid maintainability.
Apply:
-S3_OBJECTS_DELETE_LIMIT = 1000 +S3_OBJECTS_DELETE_LIMIT = 1000 # AWS max objects per ListObjectsV2 page and DeleteObjects
180-202: Normalise auth type once to avoid enum-vs-string comparison pitfalls
AwsAuthentication.typeis a Literal of enum values (strings). Normalising once keeps comparisons consistent and future-proof.Apply:
-def _create_s3_client( - region_code: str, s3_auth: AwsAuthentication, boto3_config: Optional[Config] = None -) -> boto3.client: +def _create_s3_client( + region_code: str, s3_auth: AwsAuthentication, boto3_config: Optional[Config] = None +) -> boto3.client: aws_session: Optional[boto3.Session] = None - if AwsAuthType.profile == s3_auth.type: + # Normalise the auth type to an enum for consistent comparisons + auth_type = AwsAuthType(s3_auth.type) + if AwsAuthType.profile == auth_type: aws_session = boto3.Session( profile_name=s3_auth.profile, region_name=region_code, ) - elif AwsAuthType.credentials == s3_auth.type: + elif AwsAuthType.credentials == auth_type: credentials = s3_auth.credentials aws_session = boto3.Session( aws_access_key_id=credentials.access_key_id, aws_secret_access_key=credentials.secret_access_key, region_name=region_code, aws_session_token=credentials.session_token, ) - elif AwsAuthType.env_vars == s3_auth.type or AwsAuthType.ec2 == s3_auth.type: + elif AwsAuthType.env_vars == auth_type or AwsAuthType.ec2 == auth_type: # Use default session which will use environment variables or instance role aws_session = boto3.Session(region_name=region_code) else: - raise ValueError(f"Unsupported authentication type: {s3_auth.type}") + raise ValueError(f"Unsupported authentication type: {s3_auth.type}")
320-356: Honour two-stage delete semantics: surface non-NotFound partial failuresTo ensure DB deletion doesn’t proceed after a failed archive removal, check
delete_objectsresponses. Treat “not found” as non-errors, raise on other failures.Apply:
- deletion_config = {"Objects": [{"Key": obj["Key"]} for obj in contents]} - s3_client.delete_objects(Bucket=bucket_name, Delete=deletion_config) + deletion_config = {"Objects": [{"Key": obj["Key"]} for obj in contents]} + resp = s3_client.delete_objects(Bucket=bucket_name, Delete=deletion_config) + errors = resp.get("Errors") or [] + # Ignore benign missing-key cases to keep deletions idempotent + non_ignorable = [e for e in errors if e.get("Code") not in ("NoSuchKey", "NotFound", "404")] + if non_ignorable: + failed_keys = [e.get("Key", "<unknown>") for e in non_ignorable] + raise RuntimeError( + f"S3 delete_objects reported failures for {len(non_ignorable)} object(s) under " + f"'{bucket_name}/{key_prefix}'. Example keys: {failed_keys[:3]}. " + f"Errors: {non_ignorable}" + )components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (1)
90-101: Avoid bare except; don’t mask system-exiting exceptionsCatching
Exceptionpreserves KeyboardInterrupt/SystemExit behaviour while still logging config load failures.Apply:
- except: + except Exception: logger.exception("Failed to load config.") return -1components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (4)
27-30: Decouple from wrapper: define LIST/DEL constants locallyImporting from the wrapper creates unnecessary coupling and circular-import risk. Define constants here.
Apply:
-from clp_package_utils.scripts.dataset_manager import ( - DEL_COMMAND, - LIST_COMMAND, -) +# Command/Argument Constants +LIST_COMMAND: str = "list" +DEL_COMMAND: str = "del"
253-260: Avoid bare except when loading configSame reason as above—keep system-exiting exceptions unmasked.
Apply:
- except: + except Exception: logger.exception("Failed to load config.") return -1
262-266: Scope DB fetch errors to ExceptionImprove clarity and avoid masking interrupts during DB access.
Apply:
- except: + except Exception: logger.exception("Failed to fetch datasets from the database.") return -1
91-105: Replace bare excepts with Exception to avoid masking interruptsScope exceptions in both archive and DB deletion phases so KeyboardInterrupt/SystemExit propagate correctly.
Apply:
try: _try_deleting_archives(clp_config.archive_output, dataset_archive_storage_dir) logger.info(f"Deleted archives of dataset `{dataset}`.") - except: + except Exception: logger.exception(f"Failed to delete archives of dataset `{dataset}`.") return False try: _delete_dataset_from_database(clp_config.database, dataset) logger.info(f"Deleted dataset `{dataset}` from the metadata database.") - except: + except Exception: logger.exception(f"Failed to delete dataset `{dataset}` from the metadata database.") return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py(1 hunks)components/clp-py-utils/clp_py_utils/s3_utils.py(7 hunks)components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:49.020Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
📚 Learning: 2025-08-13T14:48:49.020Z
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:49.020Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/dataset_manager.pycomponents/clp-py-utils/clp_py_utils/s3_utils.py
📚 Learning: 2025-07-03T20:10:43.789Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1050
File: components/clp-package-utils/clp_package_utils/scripts/search.py:100-106
Timestamp: 2025-07-03T20:10:43.789Z
Learning: In the current CLP codebase implementation, dataset validation using validate_dataset() is performed within the native scripts (like clp_package_utils/scripts/native/search.py) rather than at the wrapper script level, where the native scripts handle their own parameter validation.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/dataset_manager.py
📚 Learning: 2025-07-03T12:58:18.407Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1036
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py:204-211
Timestamp: 2025-07-03T12:58:18.407Z
Learning: In the CLP codebase, the validate_and_cache_dataset function in components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py uses in-place updates of the existing_datasets set parameter rather than returning a new set, as preferred by the development team.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-01-23T17:08:55.566Z
Learnt from: gibber9809
PR: y-scope/clp#672
File: components/core/src/clp_s/indexer/MySQLIndexStorage.cpp:30-38
Timestamp: 2025-01-23T17:08:55.566Z
Learning: SQL identifiers (table names, column names) in the codebase are currently from trusted sources and directly interpolated into queries. However, the team prefers to implement sanitization for robustness, even for trusted inputs.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2024-11-15T16:21:52.122Z
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:104-110
Timestamp: 2024-11-15T16:21:52.122Z
Learning: In `clp_package_utils/scripts/native/del_archives.py`, when deleting archives, the `archive` variable retrieved from the database is controlled and is always a single string without path components. Therefore, it's acceptable to skip additional validation checks for directory traversal in this context.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-08-13T15:07:37.767Z
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py:164-170
Timestamp: 2025-08-13T15:07:37.767Z
Learning: In the CLP codebase, the ArchiveOutput.get_directory() method already returns a resolved/absolute Path object because FsStorage.make_config_paths_absolute() processes the directory path during configuration loading, making additional .resolve() calls redundant.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-08-13T15:07:37.767Z
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py:164-170
Timestamp: 2025-08-13T15:07:37.767Z
Learning: In the CLP codebase, the ArchiveOutput.get_directory() method already returns a resolved Path object, so additional .resolve() calls on its return value are redundant.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
🧬 Code Graph Analysis (4)
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (5)
components/clp-py-utils/clp_py_utils/clp_config.py (8)
ArchiveOutput(573-627)Database(154-233)S3Config(451-467)StorageType(93-95)CLPConfig(702-894)get_directory(621-622)get_directory(643-644)validate_logs_dir(789-793)components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py (6)
get_archive_tags_table_name(264-265)get_archives_table_name(268-269)get_column_metadata_table_name(272-273)get_datasets_table_name(276-277)get_files_table_name(280-281)get_tags_table_name(284-285)components/clp-py-utils/clp_py_utils/s3_utils.py (1)
s3_delete_by_key_prefix(320-355)components/clp-py-utils/clp_py_utils/sql_adapter.py (1)
SQL_Adapter(60-131)components/clp-package-utils/clp_package_utils/general.py (2)
get_clp_home(106-122)load_config_file(369-395)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (4)
components/clp-py-utils/clp_py_utils/clp_config.py (4)
StorageEngine(82-84)StorageType(93-95)validate_logs_dir(789-793)get_clp_connection_params_and_type(212-233)components/clp-py-utils/clp_py_utils/s3_utils.py (1)
generate_container_auth_options(101-177)components/clp-package-utils/clp_package_utils/general.py (7)
generate_container_config(226-295)generate_container_name(125-130)generate_container_start_cmd(329-358)get_clp_home(106-122)load_config_file(369-395)validate_and_load_db_credentials_file(426-430)validate_dataset_name(556-582)components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (1)
main(198-274)
components/clp-py-utils/clp_py_utils/s3_utils.py (1)
components/clp-py-utils/clp_py_utils/clp_config.py (3)
AwsAuthentication(419-448)Config(681-682)AwsAuthType(98-102)
components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py (1)
components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py (1)
fetch_existing_datasets(184-196)
⏰ 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: lint-check (macos-15)
🔇 Additional comments (6)
components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py (2)
171-174: No stale call sites detected
All invocations ofsearch_and_schedule_new_tasksnow only passdb_connanddb_cursor. The removed argument is no longer used anywhere.
199-201: Add a defensive fallback for missing per-dataset tablesEven if deletions are rare, wrapping DB calls that rely on dataset-specific tables in a try/except will ensure the scheduler fails fast if the assumption is ever violated.
• File: components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
• Around the tag-insertion block (afterif clp_io_config.output.tags:)Suggested change:
- if clp_io_config.output.tags: + if clp_io_config.output.tags: + try: tags_table_name = get_tags_table_name(table_prefix, dataset) db_cursor.executemany( f"INSERT IGNORE INTO {tags_table_name} (tag_name) VALUES (%s)", [(tag,) for tag in clp_io_config.output.tags], ) db_conn.commit() db_cursor.execute( f"SELECT tag_id FROM {tags_table_name} WHERE tag_name IN (%s)" % ", ".join(["%s"] * len(clp_io_config.output.tags)), clp_io_config.output.tags, ) tag_ids = [tags["tag_id"] for tags in db_cursor.fetchall()] db_conn.commit() + except Exception as e: # catch missing-table (or similar) errors + logger.error("Dataset table missing, marking job as FAILED", exc_info=e) + update_compression_job_metadata( + db_cursor, + job_id, + { + "status": CompressionJobStatus.FAILED, + "status_msg": "Dataset no longer exists", + }, + ) + db_conn.commit() + continueThis ensures any unexpected missing-table error bubbles up as a controlled job failure rather than crashing the scheduler.
[optional_refactors_recommended]components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (1)
141-146: Good: mount AWS config only when required by auth modeIncluding the AWS config directory conditionally ensures S3 operations can authenticate inside the container without over-mounting.
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (3)
72-83: All-or-nothing validation before deletion is clearer UXAborting when any requested dataset doesn’t exist avoids partial deletions and matches the PR’s open decision toward fail-fast semantics.
146-157: Good: ensure trailing slash to avoid collateral deletionsThe extra “/” guard prevents removing keys with a shared prefix across datasets.
159-181: Solid safety checks before filesystem deletionThe containment check and early returns on non-existent/non-directory targets make this idempotent and safe to re-run.
| try: | ||
| clp_db_connection_params = clp_config.database.get_clp_connection_params_and_type(True) | ||
| table_prefix = clp_db_connection_params["table_prefix"] | ||
| for dataset in datasets: | ||
| validate_dataset_name(table_prefix, dataset) | ||
| except: | ||
| logger.exception("Invalid dataset name.") | ||
| return -1 | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Catch the specific validation error for clearer diagnostics
Use ValueError to scope the exception and avoid hiding unrelated issues.
Apply:
- except:
- logger.exception("Invalid dataset name.")
+ except ValueError:
+ logger.exception("Invalid dataset name.")
return -1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| clp_db_connection_params = clp_config.database.get_clp_connection_params_and_type(True) | |
| table_prefix = clp_db_connection_params["table_prefix"] | |
| for dataset in datasets: | |
| validate_dataset_name(table_prefix, dataset) | |
| except: | |
| logger.exception("Invalid dataset name.") | |
| return -1 | |
| try: | |
| clp_db_connection_params = clp_config.database.get_clp_connection_params_and_type(True) | |
| table_prefix = clp_db_connection_params["table_prefix"] | |
| for dataset in datasets: | |
| validate_dataset_name(table_prefix, dataset) | |
| except ValueError: | |
| logger.exception("Invalid dataset name.") | |
| return -1 |
🤖 Prompt for AI Agents
In components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py
around lines 118 to 126, the except block catches all exceptions which hides
unrelated errors; change the broad except to catch ValueError specifically so
only dataset validation errors are handled, log the exception details with
logger.exception or logger.error including the caught exception, and keep
returning -1 for validation failures while allowing other exceptions to
propagate.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (2)
96-98: Avoid bare except; catch concrete exceptionsCatching everything (including KeyboardInterrupt/SystemExit) makes troubleshooting harder and can mask critical errors.
Apply:
- except: + except Exception: logger.exception("Failed to load config.") return -1
114-121: Scope dataset-name validation failures to ValueError
validate_dataset_nameraises ValueError on invalid inputs. Catch that specifically; allow unexpected errors to surface.Apply:
- except: - logger.exception("Invalid dataset name.") + except ValueError: + logger.exception("Invalid dataset name.") return -1components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (3)
23-26: Decouple native script from wrapper by defining command constants locallyImporting LIST/DEL constants from the wrapper couples the native entrypoint to the wrapper and risks circular deps.
Apply:
-from clp_package_utils.scripts.dataset_manager import ( - DEL_COMMAND, - LIST_COMMAND, -) +# Command/Argument Constants +LIST_COMMAND: str = "list" +DEL_COMMAND: str = "del"
221-227: Avoid bare except when loading configLimit to Exception to prevent masking interpreter/system exceptions.
Apply:
- except: + except Exception: logger.exception("Failed to load config.") return -1
88-100: Avoid bare except; restrict to Exception in deletion flowThese two broad except blocks will swallow interrupts and system exits. Limit to Exception.
Apply:
- except: + except Exception: logger.exception(f"Failed to delete archives of dataset `{dataset}`.") return False @@ - except: + except Exception: logger.exception(f"Failed to delete dataset `{dataset}` from the metadata database.") return Falsecomponents/clp-py-utils/clp_py_utils/s3_utils.py (1)
184-202: Consider normalising auth type once for readabilityMinor readability improvement: cast to AwsAuthType once, then compare enums consistently.
Example:
- if AwsAuthType.profile == s3_auth.type: + auth_type = AwsAuthType(s3_auth.type) + if AwsAuthType.profile == auth_type: @@ - elif AwsAuthType.credentials == s3_auth.type: + elif AwsAuthType.credentials == auth_type: @@ - elif AwsAuthType.env_vars == s3_auth.type or AwsAuthType.ec2 == s3_auth.type: + elif auth_type in (AwsAuthType.env_vars, AwsAuthType.ec2): @@ - else: - raise ValueError(f"Unsupported authentication type: {s3_auth.type}") + else: + raise ValueError(f"Unsupported authentication type: {s3_auth.type}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py(1 hunks)components/clp-py-utils/clp_py_utils/clp_config.py(1 hunks)components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py(1 hunks)components/clp-py-utils/clp_py_utils/s3_utils.py(8 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:49.020Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
📚 Learning: 2025-08-13T14:48:49.020Z
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:49.020Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/dataset_manager.pycomponents/clp-py-utils/clp_py_utils/s3_utils.py
📚 Learning: 2025-07-03T12:58:18.407Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1036
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py:204-211
Timestamp: 2025-07-03T12:58:18.407Z
Learning: In the CLP codebase, the validate_and_cache_dataset function in components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py uses in-place updates of the existing_datasets set parameter rather than returning a new set, as preferred by the development team.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/dataset_manager.py
📚 Learning: 2025-07-03T20:10:43.789Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1050
File: components/clp-package-utils/clp_package_utils/scripts/search.py:100-106
Timestamp: 2025-07-03T20:10:43.789Z
Learning: In the current CLP codebase implementation, dataset validation using validate_dataset() is performed within the native scripts (like clp_package_utils/scripts/native/search.py) rather than at the wrapper script level, where the native scripts handle their own parameter validation.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/dataset_manager.py
📚 Learning: 2025-01-23T17:08:55.566Z
Learnt from: gibber9809
PR: y-scope/clp#672
File: components/core/src/clp_s/indexer/MySQLIndexStorage.cpp:30-38
Timestamp: 2025-01-23T17:08:55.566Z
Learning: SQL identifiers (table names, column names) in the codebase are currently from trusted sources and directly interpolated into queries. However, the team prefers to implement sanitization for robustness, even for trusted inputs.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2024-11-15T16:21:52.122Z
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:104-110
Timestamp: 2024-11-15T16:21:52.122Z
Learning: In `clp_package_utils/scripts/native/del_archives.py`, when deleting archives, the `archive` variable retrieved from the database is controlled and is always a single string without path components. Therefore, it's acceptable to skip additional validation checks for directory traversal in this context.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-08-13T15:07:37.767Z
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py:164-170
Timestamp: 2025-08-13T15:07:37.767Z
Learning: In the CLP codebase, the ArchiveOutput.get_directory() method already returns a resolved/absolute Path object because FsStorage.make_config_paths_absolute() processes the directory path during configuration loading, making additional .resolve() calls redundant.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-08-13T15:07:37.767Z
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py:164-170
Timestamp: 2025-08-13T15:07:37.767Z
Learning: In the CLP codebase, the ArchiveOutput.get_directory() method already returns a resolved Path object, so additional .resolve() calls on its return value are redundant.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
🧬 Code Graph Analysis (3)
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (5)
components/clp-py-utils/clp_py_utils/clp_config.py (8)
ArchiveOutput(577-631)Database(158-237)S3Config(455-471)StorageType(97-99)CLPConfig(706-898)get_clp_connection_params_and_type(216-237)get_directory(625-626)get_directory(647-648)components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py (2)
delete_dataset_from_metadata_db(264-292)get_datasets_table_name(307-308)components/clp-py-utils/clp_py_utils/s3_utils.py (1)
s3_delete_by_key_prefix(320-355)components/clp-py-utils/clp_py_utils/sql_adapter.py (1)
SQL_Adapter(60-131)components/clp-package-utils/clp_package_utils/general.py (2)
get_clp_home(107-123)load_config_file(370-396)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (3)
components/clp-py-utils/clp_py_utils/clp_config.py (4)
StorageEngine(86-88)StorageType(97-99)validate_logs_dir(793-797)get_clp_connection_params_and_type(216-237)components/clp-py-utils/clp_py_utils/s3_utils.py (1)
generate_container_auth_options(101-177)components/clp-package-utils/clp_package_utils/general.py (8)
dump_container_config(311-327)generate_container_config(227-296)generate_container_name(126-131)generate_container_start_cmd(330-359)get_clp_home(107-123)load_config_file(370-396)validate_and_load_db_credentials_file(427-431)validate_dataset_name(557-583)
components/clp-py-utils/clp_py_utils/s3_utils.py (2)
components/clp-py-utils/clp_py_utils/clp_config.py (5)
Config(685-686)S3Storage(501-530)FsStorage(482-498)AwsAuthentication(423-452)AwsAuthType(102-106)components/job-orchestration/job_orchestration/scheduler/job_config.py (1)
S3InputConfig(31-34)
⏰ 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). (2)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (6)
components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py (1)
273-281: Correct drop order with IF EXISTS; safe for idempotent rerunsDropping dataset-scoped tables in an order that respects FKs and using IF EXISTS both look correct and make the operation safe to rerun.
components/clp-py-utils/clp_py_utils/clp_config.py (1)
70-77: New action/table-name constants look good and integrate cleanlyAdding ARCHIVE_MANAGER_ACTION_NAME and COMPRESSION_TASKS_TABLE_NAME aligns with other exported names and is consistent with downstream usage.
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (1)
130-150: Good safety checks before recursive deletion on FSVerifying containment under the archives root and tolerating missing/non-dir targets aligns with the admin-tool, idempotent semantics.
components/clp-py-utils/clp_py_utils/s3_utils.py (3)
101-137: Container-type auth option generation reads clean; param rename aligns with usageThe switch to container_type and handling ARCHIVE_MANAGER_ACTION_NAME is consistent with call sites and reduces ambiguity.
180-205: S3 client creation path looks correct across auth modesRegion handling and session selection per auth type are appropriate; callers consistently pass region_code + AwsAuthentication.
320-356: Prefix-based S3 deletion with validation is solid; matches PR’s admin semanticsInput validation, pagination, and batched delete are appropriate for this tool. Skipping missing pages aligns with safe reruns.
| cmd = container_start_cmd + dataset_manager_cmd | ||
|
|
||
| proc = subprocess.run(cmd) | ||
| ret_code = proc.returncode | ||
| if 0 != ret_code: | ||
| logger.error("Dataset manager failed.") | ||
| logger.debug(f"Docker command failed: {' '.join(cmd)}") | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Handle subprocess failures explicitly to improve diagnostics
Using check=True raises on failure and lets you log the return code consistently while preserving stdout/stderr from the child process.
Apply:
- proc = subprocess.run(cmd)
- ret_code = proc.returncode
- if 0 != ret_code:
- logger.error("Dataset manager failed.")
- logger.debug(f"Docker command failed: {' '.join(cmd)}")
+ try:
+ proc = subprocess.run(cmd, check=True)
+ ret_code = proc.returncode
+ except subprocess.CalledProcessError as e:
+ ret_code = e.returncode
+ logger.error("Dataset manager failed.")
+ logger.debug(f"Docker command failed with rc={ret_code}: {' '.join(cmd)}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cmd = container_start_cmd + dataset_manager_cmd | |
| proc = subprocess.run(cmd) | |
| ret_code = proc.returncode | |
| if 0 != ret_code: | |
| logger.error("Dataset manager failed.") | |
| logger.debug(f"Docker command failed: {' '.join(cmd)}") | |
| cmd = container_start_cmd + dataset_manager_cmd | |
| try: | |
| proc = subprocess.run(cmd, check=True) | |
| ret_code = proc.returncode | |
| except subprocess.CalledProcessError as e: | |
| ret_code = e.returncode | |
| logger.error("Dataset manager failed.") | |
| logger.debug(f"Docker command failed with rc={ret_code}: {' '.join(cmd)}") |
🤖 Prompt for AI Agents
In components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py
around lines 171 to 178, the subprocess.run call does not raise on non-zero exit
and only logs a generic error; change to call subprocess.run(cmd, check=True,
capture_output=True, text=True) and wrap it in a try/except
subprocess.CalledProcessError as e: block, logging e.returncode and
e.stdout/e.stderr (or e.output) with logger.error/logger.debug so failures
include the exit code and child process output for diagnostics.
| # Remove generated files | ||
| generated_config_path_on_host.unlink() | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Guard removal of generated config file
If creation failed or the file was already removed, this will raise and hide the original outcome. Guard the cleanup.
Apply:
- generated_config_path_on_host.unlink()
+ try:
+ generated_config_path_on_host.unlink()
+ except FileNotFoundError:
+ pass
+ except Exception:
+ logger.debug(f"Failed to remove generated config at {generated_config_path_on_host}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Remove generated files | |
| generated_config_path_on_host.unlink() | |
| # Remove generated files | |
| try: | |
| generated_config_path_on_host.unlink() | |
| except FileNotFoundError: | |
| pass | |
| except Exception: | |
| logger.debug(f"Failed to remove generated config at {generated_config_path_on_host}") |
🤖 Prompt for AI Agents
In components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py
around lines 179 to 181, the call to generated_config_path_on_host.unlink() can
raise and mask earlier errors if the file was never created or already removed;
wrap the unlink in a safe guard by either checking
generated_config_path_on_host.exists() before unlinking or catching
FileNotFoundError (and only suppressing that exception) so cleanup does not
raise unexpected errors and original failures remain visible.
| try: | ||
| existing_datasets_info = _get_dataset_info(clp_config.database) | ||
| except: | ||
| logger.exception("Failed to fetch datasets from the database.") | ||
| return -1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid bare except when fetching datasets
Restrict to Exception to keep failure modes clear.
Apply:
- except:
+ except Exception:
logger.exception("Failed to fetch datasets from the database.")
return -1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| existing_datasets_info = _get_dataset_info(clp_config.database) | |
| except: | |
| logger.exception("Failed to fetch datasets from the database.") | |
| return -1 | |
| try: | |
| existing_datasets_info = _get_dataset_info(clp_config.database) | |
| except Exception: | |
| logger.exception("Failed to fetch datasets from the database.") | |
| return -1 |
🤖 Prompt for AI Agents
In
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
around lines 229 to 233, the code uses a bare except when calling
_get_dataset_info which hides non-exception failures and makes debugging
unclear; change the bare except to except Exception as e: (or a more specific
exception type if known), keep the logger.exception call (or pass e to
logger.exception for clarity) and return -1 as before so only standard
exceptions are caught and reported.
| db_cursor.execute( | ||
| f""" | ||
| DELETE FROM `{get_datasets_table_name(table_prefix)}` | ||
| WHERE name = %s | ||
| """, | ||
| (dataset,), | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Docstring should note commit responsibility and input expectations
This helper performs SQL but does not commit. Please clarify in the docstring that the caller must commit the transaction. Also, call out that dataset must already be validated/sanitised (alnum/underscore, length) since it’s used to construct identifiers.
Apply this docstring tweak:
def delete_dataset_from_metadata_db(db_cursor, table_prefix: str, dataset: str) -> None:
"""
- Deletes all tables associated with `dataset` from the metadata database.
+ Deletes all tables associated with `dataset` from the metadata database.
+
+ Notes:
+ - Caller is responsible for committing the transaction.
+ - The `dataset` parameter must be a validated identifier (alphanumeric/underscore and length-
+ constrained) as it is used to construct table names.
:param db_cursor:
:param table_prefix:
:param dataset:
"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| db_cursor.execute( | |
| f""" | |
| DELETE FROM `{get_datasets_table_name(table_prefix)}` | |
| WHERE name = %s | |
| """, | |
| (dataset,), | |
| ) | |
| def delete_dataset_from_metadata_db(db_cursor, table_prefix: str, dataset: str) -> None: | |
| """ | |
| Deletes all tables associated with `dataset` from the metadata database. | |
| Notes: | |
| - Caller is responsible for committing the transaction. | |
| - The `dataset` parameter must be a validated identifier (alphanumeric/underscore and length- | |
| constrained) as it is used to construct table names. | |
| :param db_cursor: | |
| :param table_prefix: | |
| :param dataset: | |
| """ | |
| db_cursor.execute( | |
| f""" | |
| DELETE FROM `{get_datasets_table_name(table_prefix)}` | |
| WHERE name = %s | |
| """, | |
| (dataset,), | |
| ) |
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py around lines
286-293, the helper executes a DELETE using a table name built from the
table_prefix/dataset but does not commit and uses the dataset to construct an
identifier; update the function docstring to (1) state explicitly that this
function does not call commit and the caller is responsible for
committing/rolling back the transaction, and (2) require that the dataset
argument is pre-validated/sanitized (only alphanumeric and underscores, within a
specified max length) before calling because it is interpolated into an
identifier—document input expectations and possible injection risk so callers
know to validate prior to use.
|
Retested on fs, s3-profile and s3-credentials |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (5)
87-103: Replace bare except withexcept ExceptionCatching BaseException masks interrupts and system exits.
try: _try_deleting_archives(clp_config.archive_output, dataset_archive_storage_dir) logger.info(f"Deleted archives of dataset `{dataset}`.") - except: + except Exception: logger.exception(f"Failed to delete archives of dataset `{dataset}`.") return False try: _delete_dataset_from_database(clp_config.database, dataset) logger.info(f"Deleted dataset `{dataset}` from the metadata database.") - except: + except Exception: logger.exception(f"Failed to delete dataset `{dataset}` from the metadata database.") return False
220-227: Avoid bare except while loading configCatches system-exiting exceptions.
- except: + except Exception: logger.exception("Failed to load config.") return -1
229-233: Avoid bare except when fetching datasetsLimit to Exception to keep failure modes clear.
- except: + except Exception: logger.exception("Failed to fetch datasets from the database.") return -1
31-167: Optional: reorder functions for top-down readabilityPlace main first, then high-level handlers, then lower-level helpers. Matches prior team preference.
23-26: Avoid brittle cross-module coupling: define CLI constants locallyImporting LIST/DEL from the wrapper risks circular imports and tight coupling. Make this native script self-contained.
Apply:
-from clp_package_utils.scripts.dataset_manager import ( - DEL_COMMAND, - LIST_COMMAND, -) +# Command/Argument Constants +LIST_COMMAND: str = "list" +DEL_COMMAND: str = "del"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-13T14:48:49.020Z
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:49.020Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-07-03T12:58:18.407Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1036
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py:204-211
Timestamp: 2025-07-03T12:58:18.407Z
Learning: In the CLP codebase, the validate_and_cache_dataset function in components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py uses in-place updates of the existing_datasets set parameter rather than returning a new set, as preferred by the development team.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-07-03T20:10:43.789Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1050
File: components/clp-package-utils/clp_package_utils/scripts/search.py:100-106
Timestamp: 2025-07-03T20:10:43.789Z
Learning: In the current CLP codebase implementation, dataset validation using validate_dataset() is performed within the native scripts (like clp_package_utils/scripts/native/search.py) rather than at the wrapper script level, where the native scripts handle their own parameter validation.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-01-23T17:08:55.566Z
Learnt from: gibber9809
PR: y-scope/clp#672
File: components/core/src/clp_s/indexer/MySQLIndexStorage.cpp:30-38
Timestamp: 2025-01-23T17:08:55.566Z
Learning: SQL identifiers (table names, column names) in the codebase are currently from trusted sources and directly interpolated into queries. However, the team prefers to implement sanitization for robustness, even for trusted inputs.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2024-11-15T16:21:52.122Z
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:104-110
Timestamp: 2024-11-15T16:21:52.122Z
Learning: In `clp_package_utils/scripts/native/del_archives.py`, when deleting archives, the `archive` variable retrieved from the database is controlled and is always a single string without path components. Therefore, it's acceptable to skip additional validation checks for directory traversal in this context.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-08-13T15:07:37.767Z
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py:164-170
Timestamp: 2025-08-13T15:07:37.767Z
Learning: In the CLP codebase, the ArchiveOutput.get_directory() method already returns a resolved/absolute Path object because FsStorage.make_config_paths_absolute() processes the directory path during configuration loading, making additional .resolve() calls redundant.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
📚 Learning: 2025-08-13T15:07:37.767Z
Learnt from: haiqi96
PR: y-scope/clp#1144
File: components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py:164-170
Timestamp: 2025-08-13T15:07:37.767Z
Learning: In the CLP codebase, the ArchiveOutput.get_directory() method already returns a resolved Path object, so additional .resolve() calls on its return value are redundant.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
🔇 Additional comments (5)
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (5)
105-116: Dispatch logic looks correctFS first, then S3, with a clear error for unsupported types. Matches the PR’s stated storage coverage.
155-165: DB deletion and commit flow LGTMUsing the adapter and committing after metadata deletion fits the two-phase deletion design.
171-200: CLI structure is clear and consistentGood help texts, subcommands, and flags. Description accurately reflects functionality.
142-153: Verified s3_delete_by_key_prefix signature and call orderThe call to s3_delete_by_key_prefix passes arguments as (region_code, bucket_name, key_prefix, s3_auth), which exactly matches its definition:
def s3_delete_by_key_prefix( region_code: str, bucket_name: str, key_prefix: str, s3_auth: AwsAuthentication ) -> None:No changes required.
120-129: No fallback needed for Path.is_relative_to() – Python 3.9+ is guaranteedThe
components/clp-package-utils/pyproject.tomldeclares[project] requires-python = ">= 3.9"and CI workflows exercise Python 3.10 and 3.11. Since
Path.is_relative_to()was introduced in 3.9, it’s safe to use it directly. The suggested try/except fallback for <3.9 compatibility isn’t necessary here.
| LIST_COMMAND, | ||
| ) | ||
|
|
||
| logger: logging.Logger = logging.getLogger(__file__) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use name for logger to preserve module hierarchy
Using file creates an absolute-path logger name, which is atypical and less filterable.
-logger: logging.Logger = logging.getLogger(__file__)
+logger: logging.Logger = logging.getLogger(__name__)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger: logging.Logger = logging.getLogger(__file__) | |
| logger: logging.Logger = logging.getLogger(__name__) |
🤖 Prompt for AI Agents
In
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
around line 28, the logger is created with logging.getLogger(__file__) which
yields an absolute-path name; change it to logging.getLogger(__name__) to
preserve the module hierarchy and make logger names filterable. Replace the
getLogger argument from __file__ to __name__ and run tests/lint to ensure
imports and logging behavior remain correct.
| db_cursor.execute( | ||
| f"SELECT name, archive_storage_directory FROM `{get_datasets_table_name(table_prefix)}`" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Deterministic LIST output: sort results in SQL
Ordering by name improves UX and makes outputs stable across runs.
- db_cursor.execute(
- f"SELECT name, archive_storage_directory FROM `{get_datasets_table_name(table_prefix)}`"
- )
+ db_cursor.execute(
+ f"SELECT name, archive_storage_directory FROM `{get_datasets_table_name(table_prefix)}` ORDER BY name"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| db_cursor.execute( | |
| f"SELECT name, archive_storage_directory FROM `{get_datasets_table_name(table_prefix)}`" | |
| ) | |
| db_cursor.execute( | |
| f"SELECT name, archive_storage_directory FROM `{get_datasets_table_name(table_prefix)}` ORDER BY name" | |
| ) |
🤖 Prompt for AI Agents
In
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
around lines 45 to 47, the SQL SELECT that lists datasets is not ordered,
causing non-deterministic output; modify the query to include an ORDER BY name
(e.g., ORDER BY name ASC) so results are consistently sorted by dataset name,
ensuring deterministic and stable listing behavior.
| def _handle_list_datasets(datasets: Dict[str, str]) -> int: | ||
| logger.info(f"Found {len(datasets)} datasets.") | ||
| for dataset_name in datasets.keys(): | ||
| logger.info(dataset_name) | ||
| return 0 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Pythonic iteration over dict keys
No need to call .keys(); direct iteration is clearer.
- for dataset_name in datasets.keys():
- logger.info(dataset_name)
+ for dataset in datasets:
+ logger.info(dataset)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _handle_list_datasets(datasets: Dict[str, str]) -> int: | |
| logger.info(f"Found {len(datasets)} datasets.") | |
| for dataset_name in datasets.keys(): | |
| logger.info(dataset_name) | |
| return 0 | |
| def _handle_list_datasets(datasets: Dict[str, str]) -> int: | |
| logger.info(f"Found {len(datasets)} datasets.") | |
| for dataset in datasets: | |
| logger.info(dataset) | |
| return 0 |
🤖 Prompt for AI Agents
In
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
around lines 52 to 56, the for-loop unnecessarily calls .keys() on the dict;
iterate directly over the dict instead (for dataset_name in datasets:) and keep
the logger.info(dataset_name) and return unchanged.
| def _handle_del_datasets( | ||
| clp_config: CLPConfig, | ||
| parsed_args: argparse.Namespace, | ||
| existing_datasets_info: Dict[str, str], | ||
| ): | ||
| if len(existing_datasets_info) == 0: | ||
| logger.warning("No datasets exist.") | ||
| return 0 | ||
|
|
||
| datasets_to_delete: Dict[str, str] = {} | ||
| if parsed_args.del_all: | ||
| datasets_to_delete = existing_datasets_info | ||
| else: | ||
| datasets = parsed_args.datasets | ||
| for dataset in datasets: | ||
| if dataset not in existing_datasets_info: | ||
| logger.error(f"Dataset `{dataset}` doesn't exist. Aborting deletion.") | ||
| return -1 | ||
|
|
||
| datasets_to_delete = {dataset: existing_datasets_info[dataset] for dataset in datasets} | ||
|
|
||
| for dataset, dataset_archive_storage_dir in datasets_to_delete.items(): | ||
| if not _delete_dataset(clp_config, dataset, dataset_archive_storage_dir): | ||
| return -1 | ||
|
|
||
| return 0 | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Guard against no-op deletions when no datasets are specified
Running del without arguments and without --all silently does nothing and returns success. Fail fast with a clear message.
datasets = parsed_args.datasets
+ if not datasets:
+ logger.error("No datasets specified. Provide at least one dataset or use --all.")
+ return -1
for dataset in datasets:
if dataset not in existing_datasets_info:
logger.error(f"Dataset `{dataset}` doesn't exist. Aborting deletion.")
return -1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _handle_del_datasets( | |
| clp_config: CLPConfig, | |
| parsed_args: argparse.Namespace, | |
| existing_datasets_info: Dict[str, str], | |
| ): | |
| if len(existing_datasets_info) == 0: | |
| logger.warning("No datasets exist.") | |
| return 0 | |
| datasets_to_delete: Dict[str, str] = {} | |
| if parsed_args.del_all: | |
| datasets_to_delete = existing_datasets_info | |
| else: | |
| datasets = parsed_args.datasets | |
| for dataset in datasets: | |
| if dataset not in existing_datasets_info: | |
| logger.error(f"Dataset `{dataset}` doesn't exist. Aborting deletion.") | |
| return -1 | |
| datasets_to_delete = {dataset: existing_datasets_info[dataset] for dataset in datasets} | |
| for dataset, dataset_archive_storage_dir in datasets_to_delete.items(): | |
| if not _delete_dataset(clp_config, dataset, dataset_archive_storage_dir): | |
| return -1 | |
| return 0 | |
| def _handle_del_datasets( | |
| clp_config: CLPConfig, | |
| parsed_args: argparse.Namespace, | |
| existing_datasets_info: Dict[str, str], | |
| ): | |
| if len(existing_datasets_info) == 0: | |
| logger.warning("No datasets exist.") | |
| return 0 | |
| datasets_to_delete: Dict[str, str] = {} | |
| if parsed_args.del_all: | |
| datasets_to_delete = existing_datasets_info | |
| else: | |
| datasets = parsed_args.datasets | |
| if not datasets: | |
| logger.error("No datasets specified. Provide at least one dataset or use --all.") | |
| return -1 | |
| for dataset in datasets: | |
| if dataset not in existing_datasets_info: | |
| logger.error(f"Dataset `{dataset}` doesn't exist. Aborting deletion.") | |
| return -1 | |
| datasets_to_delete = {dataset: existing_datasets_info[dataset] for dataset in datasets} | |
| for dataset, dataset_archive_storage_dir in datasets_to_delete.items(): | |
| if not _delete_dataset(clp_config, dataset, dataset_archive_storage_dir): | |
| return -1 | |
| return 0 |
🤖 Prompt for AI Agents
In
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py
around lines 59 to 85, the handler treats a call to `del` with no arguments (and
without `--all`) as a successful no-op; add a guard that if parsed_args.del_all
is False and parsed_args.datasets is empty or None, log an error stating that no
datasets were specified and return -1 to fail fast; otherwise proceed with the
existing validation and deletion logic.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat(package): Add `dataset-manager` scripts to support listing datasets, and deleting them entirely.
dataset-manager scripts to support datasets manangement.dataset-manager scripts to support listing datasets, and deleting them entirely.
| existing_datasets = fetch_existing_datasets( | ||
| db_cursor, clp_metadata_db_connection_config["table_prefix"] | ||
| ) |
There was a problem hiding this comment.
In the CLP Text package, the compression scheduler crashes on any job submission:
2025-08-17 23:31:57,128 compression_scheduler [INFO] Starting compression scheduler
2025-08-17 23:31:57,130 compression_scheduler [ERROR] Error in scheduling.
Traceback (most recent call last):
File "/opt/clp/lib/python3/site-
packages/job_orchestration/scheduler/compress/compression_scheduler.py", line 430, in main
search_and_schedule_new_tasks(
File "/opt/clp/lib/python3/site-
packages/job_orchestration/scheduler/compress/compression_scheduler.py", line 171, in
search_and_schedule_new_tasks
existing_datasets = fetch_existing_datasets(
File "/opt/clp/lib/python3/site-packages/clp_py_utils/clp_metadata_db_utils.py", line 194, in
fetch_existing_datasets
db_cursor.execute(f"SELECT name FROM `{get_datasets_table_name(table_prefix)}`")
mariadb.ProgrammingError: Table 'clp-db.clp_datasets' doesn't exist
steps to reproduce
cd clp-package/sbin
./start-clp.sh
./compress.sh ~/samples/hive-24h
expected
job completes successfully with speed displayed: e.g.,
junhao@ASUS-X870E:~/workspace/docs-clp/build/clp-package/sbin$ ./compress.sh ~/samples/hive-24hr/
2025-08-18T01:28:52.264 INFO [compress] Compression job 1 submitted.
2025-08-18T01:28:57.353 INFO [compress] Compressed 79.16MB into 1.74MB (45.42x). Speed: 60.15MB/s.
2025-08-18T01:28:58.858 INFO [compress] Compressed 1.08GB into 28.37MB (38.91x). Speed: 391.22MB/s.
2025-08-18T01:28:59.363 INFO [compress] Compressed 1.58GB into 41.66MB (38.82x). Speed: 486.25MB/s.
2025-08-18T01:29:00.371 INFO [compress] Compression finished.
2025-08-18T01:29:00.371 INFO [compress] Compressed 1.99GB into 45.22MB (45.03x). Speed: 512.79MB/s.
as proposed by @haiqi96 , adding specifical handling for storage engine being "clp" resolved the issue. e.g., apply below git patch:
Index: components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py b/components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
--- a/components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py (revision e346d0f2cea970ba92bb74f20088405f3218ccf0)
+++ b/components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py (date 1755479177205)
@@ -157,20 +157,22 @@
db_conn,
db_cursor,
clp_metadata_db_connection_config: Dict[str, Any],
- clp_archive_output: ArchiveOutput,
+ clp_config: CLPConfig,
):
"""
For all jobs with PENDING status, splits the job into tasks and schedules them.
:param db_conn:
:param db_cursor:
:param clp_metadata_db_connection_config:
- :param clp_archive_output:
+ :param clp_config:
"""
global scheduled_jobs
- existing_datasets = fetch_existing_datasets(
- db_cursor, clp_metadata_db_connection_config["table_prefix"]
- )
+ existing_datasets: Set[str] = set()
+ if StorageEngine.CLP_S == clp_config.package.storage_engine:
+ existing_datasets = fetch_existing_datasets(
+ db_cursor, clp_metadata_db_connection_config["table_prefix"]
+ )
logger.debug("Search and schedule new tasks")
@@ -193,7 +195,7 @@
db_cursor,
table_prefix,
dataset,
- clp_archive_output,
+ clp_config.archive_output,
)
# NOTE: This assumes we never delete a dataset when compression jobs are being scheduled
@@ -431,7 +433,7 @@
db_conn,
db_cursor,
clp_metadata_db_connection_config,
- clp_config.archive_output,
+ clp_config,
)
poll_running_jobs(db_conn, db_cursor)
time.sleep(clp_config.compression_scheduler.jobs_poll_delay)
@coderabbitai please help create an issue to track this. Refer to .github/ISSUE_TEMPLATE/bug-report.yml for the issue report's format
There was a problem hiding this comment.
🧩 Analysis chain
I'll help you create an issue to track this bug. Let me first check the bug report template format and then create the issue.
@junhaoliao I've created a GitHub issue to track this bug. The issue documents the compression scheduler crash in the CLP Text package, including your reproduction steps, the error trace, and the proposed solution from @haiqi96.
The root cause is that the code unconditionally tries to fetch existing datasets from the clp_datasets table, but this table only exists for CLP-S storage engine. For CLP Text package, this causes a database error since the table doesn't exist.
The proposed fix makes sense - checking the storage engine type before attempting to fetch existing datasets, which aligns with the dataset management functionality being specific to CLP-S.
🏁 Script executed:
#!/bin/bash
# Fetch the bug report template to understand the required format
cat .github/ISSUE_TEMPLATE/bug-report.yml
Length of output: 1221
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the compression scheduler crash bug
gh issue create \
--title "Compression scheduler crashes on job submission in CLP Text package due to missing datasets table" \
--body "## Bug Report
### Description
The compression scheduler crashes when processing any job submission in the CLP Text package due to attempting to query a non-existent \`clp_datasets\` table.
### Environment
- **CLP Version**: Current main branch
- **Storage Engine**: CLP Text (not CLP-S)
- **Database**: MariaDB
### Steps to Reproduce
1. \`cd clp-package/sbin\`
2. \`./start-clp.sh\`
3. \`./compress.sh ~/samples/hive-24h\`
### Expected Behavior
Job should complete successfully with compression statistics displayed:
\`\`\`
2025-08-18T01:28:52.264 INFO [compress] Compression job 1 submitted.
2025-08-18T01:28:57.353 INFO [compress] Compressed 79.16MB into 1.74MB (45.42x). Speed: 60.15MB/s.
2025-08-18T01:28:58.858 INFO [compress] Compressed 1.08GB into 28.37MB (38.91x). Speed: 391.22MB/s.
2025-08-18T01:28:59.363 INFO [compress] Compressed 1.58GB into 41.66MB (38.82x). Speed: 486.25MB/s.
2025-08-18T01:29:00.371 INFO [compress] Compression finished.
2025-08-18T01:29:00.371 INFO [compress] Compressed 1.99GB into 45.22MB (45.03x). Speed: 512.79MB/s.
\`\`\`
### Actual Behavior
Compression scheduler crashes with the following error:
\`\`\`
2025-08-17 23:31:57,128 compression_scheduler [INFO] Starting compression scheduler
2025-08-17 23:31:57,130 compression_scheduler [ERROR] Error in scheduling.
Traceback (most recent call last):
File \"/opt/clp/lib/python3/site-packages/job_orchestration/scheduler/compress/compression_scheduler.py\", line 430, in main
search_and_schedule_new_tasks(
File \"/opt/clp/lib/python3/site-packages/job_orchestration/scheduler/compress/compression_scheduler.py\", line 171, in search_and_schedule_new_tasks
existing_datasets = fetch_existing_datasets(
File \"/opt/clp/lib/python3/site-packages/clp_py_utils/clp_metadata_db_utils.py\", line 194, in fetch_existing_datasets
db_cursor.execute(f\"SELECT name FROM \\\`{get_datasets_table_name(table_prefix)}\\\`\")
mariadb.ProgrammingError: Table 'clp-db.clp_datasets' doesn't exist
\`\`\`
### Root Cause
The code unconditionally calls \`fetch_existing_datasets\` which tries to query the \`clp_datasets\` table. However, this table only exists for CLP-S storage engine, not for CLP Text package.
### Proposed Solution
Add storage engine check before fetching existing datasets, as suggested by @haiqi96:
- Only call \`fetch_existing_datasets\` when storage engine is CLP-S
- Initialize \`existing_datasets\` as empty set for other storage engines
### Additional Context
- Issue affects: \`components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py\`
- Related PR: https://github.com/y-scope/clp/pull/1144
- Comment: https://github.com/y-scope/clp/pull/1144#discussion_r2281128455
### Reporter
@junhaoliao" \
--assignee junhaoliao
Length of output: 2914
@junhaoliao Perfect! I've successfully created issue #1214 to track this bug. The issue includes:
- Detailed description of the compression scheduler crash
- Environment details (CLP Text package, MariaDB)
- Step-by-step reproduction instructions
- Expected vs actual behavior with full error traceback
- Root cause analysis explaining why the
clp_datasetstable doesn't exist for CLP Text - Proposed solution from @haiqi96's suggestion
- Backlinks to this PR and comment for context
The issue is assigned to you and properly documents that the datasets table functionality is specific to CLP-S storage engine, while CLP Text package doesn't have this table, causing the unconditional fetch_existing_datasets call to fail.
This should help track the fix needed to add the storage engine check before attempting to fetch existing datasets.
(\ /)
( ._.)
o_(")(") *thoughtful rabbit creating documentation*
🧬 Code Graph Analysis Results
-
File: components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
Line range: around 157-171
Snippet:
existing_datasets = fetch_existing_datasets(
db_cursor, clp_metadata_db_connection_config["table_prefix"]
)Rationale: This is the code path that fetches existing datasets and can crash if the datasets table does not exist for the current storage engine.
-
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py
Line range: 184-196
Snippet:
def fetch_existing_datasets(
db_cursor,
table_prefix: str,
) -> Set[str]:
"""
Gets the names of all existing datasets.:param db_cursor: :param table_prefix: """ db_cursor.execute(f"SELECT name FROM `{get_datasets_table_name(table_prefix)}`") rows = db_cursor.fetchall() return {row["name"] for row in rows}Rationale: This is the function that queries the datasets table and would fail if the table doesn’t exist.
-
File: components/clp-py-utils/clp_py_utils/initialize-clp-metadata-db.py
Line range: 31-65
Snippet:
def main(argv):
args_parser = argparse.ArgumentParser(description="Sets up CLP's metadata tables.")
args_parser.add_argument("--config", required=True, help="Database config file.")
args_parser.add_argument(
"--storage-engine",
type=str,
choices=[engine.value for engine in StorageEngine],
required=True,
help="Storage engine to create tables for.",
)
parsed_args = args_parser.parse_args(argv[1:])config_file_path = Path(parsed_args.config) storage_engine = StorageEngine(parsed_args.storage_engine) try: database_config = Database.parse_obj(read_yaml_config_file(config_file_path)) if database_config is None: raise ValueError(f"Database configuration file '{config_file_path}' is empty.") sql_adapter = SQL_Adapter(database_config) clp_db_connection_params = database_config.get_clp_connection_params_and_type(True) table_prefix = clp_db_connection_params["table_prefix"] with closing(sql_adapter.create_connection(True)) as metadata_db, closing( metadata_db.cursor(dictionary=True) ) as metadata_db_cursor: if StorageEngine.CLP_S == storage_engine: create_datasets_table(metadata_db_cursor, table_prefix) else: create_metadata_db_tables(metadata_db_cursor, table_prefix) metadata_db.commit() except: logger.exception("Failed to create clp metadata tables.") return -1Rationale: Shows how the datasets table is created conditionally based on the storage engine (CLP_S vs others).
-
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py
Line range: 148-181
Snippet:
def add_dataset(
db_conn,
db_cursor,
table_prefix: str,
dataset_name: str,
archive_output: ArchiveOutput,
) -> None:
"""
Inserts a new dataset into thedatasetstable and creates the corresponding standard set of
tables for CLP's metadata.:param db_conn: :param db_cursor: The database cursor to execute the table row insertion. :param table_prefix: A string to prepend to the table name. :param dataset_name: :param archive_output: """ archive_storage_directory: Path if StorageType.S3 == archive_output.storage.type: s3_config = archive_output.storage.s3_config archive_storage_directory = Path(s3_config.key_prefix) else: archive_storage_directory = archive_output.get_directory() query = f"""INSERT INTO `{get_datasets_table_name(table_prefix)}` (name, archive_storage_directory) VALUES (%s, %s) """ db_cursor.execute( query, (dataset_name, str(archive_storage_directory / dataset_name)), ) create_metadata_db_tables(db_cursor, table_prefix, dataset_name) db_conn.commit()Rationale: Shows how a dataset is recorded and how the archive_output affects the archivedir used for the dataset.
-
File: components/clp-py-utils/clp_py_utils/sql_adapter.py
Line range: 216-237
Snippet:
def get_clp_connection_params_and_type(self, disable_localhost_socket_connection: bool = False):
self.ensure_credentials_loaded()host = self.host if disable_localhost_socket_connection and "localhost" == self.host: host = "127.0.0.1" connection_params_and_type = { # NOTE: clp-core does not distinguish between mysql and mariadb "type": "mysql", "host": host, "port": self.port, "username": self.username, "password": self.password, "name": self.name, "table_prefix": CLP_METADATA_TABLE_PREFIX, "compress": self.compress, "autocommit": self.auto_commit, } if self.ssl_cert: connection_params_and_type["ssl_cert"] = self.ssl_cert return connection_params_and_typeRationale: Illustrates how connection params and table prefix are derived for CLP metadata, which affects whether the datasets table exists depending on storage engine.
-
File: components/clp-py-utils/clp_py_utils/clp_config.py
Line range: 86-88
Snippet:
class StorageEngine(KebabCaseStrEnum):
CLP = auto()
CLP_S = auto()Rationale: Shows the CLP_S storage engine value used in conditional logic (e.g., whether to create/use datasets table).
-
File: components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py
Line range: around 431-433
Snippet:
(patch context illustrating where the call to search_and_schedule_new_tasks ultimately passes the CLP config)
...Rationale: This area in the file is where a final call would pass in a CLPConfig object to influence how tasks are scheduled, relevant to the proposed fix in the user comment.
🧠 Learnings used
Learnt from: junhaoliao
PR: y-scope/clp#1152
File: components/clp-package-utils/clp_package_utils/scripts/start_clp.py:613-613
Timestamp: 2025-08-08T06:59:42.436Z
Learning: In components/clp-package-utils/clp_package_utils/scripts/start_clp.py, generic_start_scheduler sets CLP_LOGGING_LEVEL using clp_config.query_scheduler.logging_level for both schedulers; compression scheduler should use its own logging level. Tracking via an issue created from PR #1152 discussion.
Learnt from: haiqi96
PR: y-scope/clp#651
File: components/clp-package-utils/clp_package_utils/scripts/compress.py:0-0
Timestamp: 2025-01-16T16:58:43.190Z
Learning: In the clp-package compression flow, path validation and error handling is performed at the scheduler level rather than in the compress.py script to maintain simplicity and avoid code duplication.
3 participants
Description
This PR adds a managment script for dataset that support listing and deleting existing dataset. The script is based on the following assumption:
The script supports the following operation
Some behavior to be decided:
Note: this PR also updates the dataset logic in compression scheduler, because the current implemetation assumes that dataset are never removed.
The current implemetation let compression scheduler poll the dataset for every new compression job, and assumes that no dataset will be deleted when a job is being scheduled.
Some example command and output
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Refactor