Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

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.

components/clp-package-utils/clp_package_utils/scripts/archive_manager.py (1)

159-162: Centralize logging level setup into a small helper to avoid duplication

This pattern will recur across wrapper scripts. Extract it into a helper (module-local now; shared later) to keep behaviour consistent.

Apply this diff to simplify the call site:

-    if parsed_args.debug:
-        logger.setLevel(logging.DEBUG)
-    else:
-        logger.setLevel(logging.INFO)
+    set_logging_level(parsed_args.debug)

Outside the selected range, add this helper near the top of the module:

def set_logging_level(debug: bool) -> None:
    """Sets module logger level based on a debug flag."""
    logger.setLevel(logging.DEBUG if debug else logging.INFO)

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py (4)

254-260: Fix invalid type hint/default for end_ts

end_ts: int = typing.Optional[int] is a bug. You likely meant Optional[int] with a default of None. This currently sets the default to the type object, which breaks your None checks and is semantically wrong.

-def _find_archives(
-    archives_dir: Path,
-    database_config: Database,
-    dataset: typing.Optional[str],
-    begin_ts: int,
-    end_ts: int = typing.Optional[int],
-) -> int:
+def _find_archives(
+    archives_dir: Path,
+    database_config: Database,
+    dataset: typing.Optional[str],
+    begin_ts: int,
+    end_ts: typing.Optional[int] = None,
+) -> int:

---

373-387: DELETE … IN clause is broken; placeholders aren’t bound

ids_list_string is built with quoted "%s" and you’re not passing parameters to execute(), so the driver will see literal strings '%s' instead of binding archive_ids. This will fail silently or delete nothing.

-            ids_list_string: str = ", ".join(["'%s'"] * len(archive_ids))
+            placeholders: str = ", ".join(["%s"] * len(archive_ids))

             db_cursor.execute(
                 f"""
                 DELETE FROM `{get_files_table_name(table_prefix, dataset)}`
-                WHERE archive_id in ({ids_list_string})
+                WHERE archive_id in ({placeholders})
                 """
-            )
+            , archive_ids)

             db_cursor.execute(
                 f"""
                 DELETE FROM `{get_archive_tags_table_name(table_prefix, dataset)}`
-                WHERE archive_id in ({ids_list_string})
+                WHERE archive_id in ({placeholders})
                 """
-            )
+            , archive_ids)

---

404-414: Harden disk deletion and keep going on partial failures

A single rmtree failure aborts the whole operation. Wrap in try/except to log and continue (or return -1 if you prefer to fail-fast).

-    for archive_id in archive_ids:
-        archive_path = archive_output_dir / archive_id
-        if not archive_path.is_dir():
-            logger.warning(f"Archive {archive_id} is not a directory. Skipping deletion.")
-            continue
-
-        logger.info(f"Deleting archive {archive_id} from disk.")
-        shutil.rmtree(archive_path)
+    for archive_id in archive_ids:
+        archive_path = archive_output_dir / archive_id
+        if not archive_path.exists():
+            logger.warning(f"Archive {archive_id} not found on disk. Skipping deletion.")
+            continue
+        if not archive_path.is_dir():
+            logger.warning(f"Archive {archive_id} is not a directory. Skipping deletion.")
+            continue
+        logger.info(f"Deleting archive {archive_id} from disk.")
+        try:
+            shutil.rmtree(archive_path)
+        except Exception:
+            logger.exception(f"Failed to delete archive {archive_id} from disk.")

---

261-270: Docstring return value doesn’t match implementation

Docstring says “1 on failure” but code returns -1 on failure. Update for consistency.

-    :return: 0 on success, 1 on failure.
+    :return: 0 on success, -1 on failure.

components/clp-package-utils/clp_package_utils/scripts/archive_manager.py (1)

1-285: Fix inconsistent debug/verbose flags in archive manager scripts

The wrapper and native archive-manager CLIs are inconsistent: both define a --verbose flag but the code checks parsed_args.debug, and the wrapper forwards --debug to the native module. This will cause incorrect behaviour or runtime errors.

Files/locations found (from repo search):

• components/clp-package-utils/clp_package_utils/scripts/archive_manager.py
  • "--verbose" arg defined (line ~68)
  • checks parsed_args.debug (line ~157)
  • forwards/appends "--debug" to the native command (line ~237-238)
• components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py
  • "--verbose" arg defined (line ~100)
  • checks parsed_args.debug (line ~186)

Minimal, safe fixes

• Make the flag name and attribute consistent across both scripts. E.g. add the flag as aliases and set dest="debug":
  • args_parser.add_argument("--debug", "--verbose", "-v", dest="debug", action="store_true", help="Enable debug logging.")
    This preserves both flag names for users while ensuring parsed_args.debug exists and the wrapper's forwarded "--debug" is accepted by the native script.
• Alternatively, consistently use parsed_args.verbose everywhere and forward "--verbose" from the wrapper — but choose one approach across both scripts.

Recommended refactor (optional, but reduces drift)

• Centralize logging/CLI behaviour:
  • add_logging_options(parser): adds the common --debug/--verbose flags (with a consistent dest)
  • set_logging_level(parsed_args) or set_logging_level(debug: bool)
  • run_docker_cmd(cmd: list[str], keep_artifacts_when_verbose: bool): runs docker + handles cleanup/logging
    This will remove duplication and prevent future flag/name drift.

components/clp-package-utils/clp_package_utils/scripts/archive_manager.py (1)

268-281: Propagate child exit code, pre-run DEBUG log, and robust cleanup

Current code loses the inner exit code (always -1 on failure), only logs the command post-failure, and unlink can raise. Improve diagnosability and robustness without reintroducing stack traces. This repeats earlier feedback.

-    ret_code: int
-    try:
-        subprocess.run(cmd, check=True)
-        ret_code = 0
-    except Exception:
-        logger.error("Archive manager failed.")
-        logger.debug(f"Docker command failed: {' '.join(cmd)}")
-        ret_code = -1
-    finally:
-        # Remove generated files
-        generated_config_path_on_host.unlink()
-
-    return ret_code
+    ret_code: int
+    # Emit the full container command when debugging is enabled.
+    if logger.isEnabledFor(logging.DEBUG):
+        # Use proper shell-quoting for readability
+        logger.debug(f"Running Docker command: {shlex.join(cmd)}")
+    try:
+        proc = subprocess.run(cmd)
+        ret_code = proc.returncode
+        if 0 != ret_code:
+            logger.error("Archive manager failed.")
+            logger.debug(f"Docker command failed: {shlex.join(cmd)}")
+    except Exception as e:
+        # Failure to start the process (not an inner failure).
+        logger.error(f"Failed to start Docker command: {e}")
+        logger.debug(f"Docker command: {shlex.join(cmd)}")
+        ret_code = -1
+    finally:
+        # Remove generated files; tolerate if already removed.
+        try:
+            # Keep the generated config when verbose for easier debugging.
+            if not parsed_args.verbose:
+                generated_config_path_on_host.unlink()
+        except FileNotFoundError:
+            pass
+
+    return ret_code

Outside the selected range, add:

import shlex

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

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: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.

components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py (7)

197-199: Avoid bare except; don’t catch BaseException

Catching BaseException will swallow SystemExit/KeyboardInterrupt. Limit to Exception.

-    except:
+    except Exception:
         logger.exception("Failed to load config.")
         return -1

---

253-259: Incorrect type/default for end_ts; make begin_ts/end_ts Optional[int] with None defaults

The current signature sets a typing object as the default value and types begin_ts as int, while your CLI makes it optional. This causes inconsistent behaviour and type hints.

-def _find_archives(
-    archives_dir: Path,
-    database_config: Database,
-    dataset: typing.Optional[str],
-    begin_ts: int,
-    end_ts: int = typing.Optional[int],
-) -> int:
+def _find_archives(
+    archives_dir: Path,
+    database_config: Database,
+    dataset: typing.Optional[str],
+    begin_ts: typing.Optional[int] = None,
+    end_ts: typing.Optional[int] = None,
+) -> int:

---

260-269: Docstring return code is inconsistent with implementation

Doc says “1 on failure” but function returns -1 on failure; align the docstring or the code. The rest of the module uses -1 for failures.

-    :return: 0 on success, 1 on failure.
+    :return: 0 on success, -1 on failure.

---

283-294: Handle optional begin_ts/end_ts correctly in the SELECT; avoid passing None as a bound param

Right now the query always binds begin_ts, which may be None; that can yield no rows or DB errors. Build WHERE clauses conditionally.

-            query_params: typing.List[int] = [begin_ts]
-            query: str = (
-                f"""
-                SELECT id FROM `{get_archives_table_name(table_prefix, dataset)}`
-                WHERE begin_timestamp >= %s
-                """
-            )
-            if end_ts is not None:
-                query += " AND end_timestamp <= %s"
-                query_params.append(end_ts)
+            query_params: typing.List[typing.Any] = []
+            query: str = f"SELECT id FROM `{get_archives_table_name(table_prefix, dataset)}`"
+            where_clauses: typing.List[str] = []
+            if begin_ts is not None:
+                where_clauses.append("begin_timestamp >= %s")
+                query_params.append(begin_ts)
+            if end_ts is not None:
+                where_clauses.append("end_timestamp <= %s")
+                query_params.append(end_ts)
+            if where_clauses:
+                query += " WHERE " + " AND ".join(where_clauses)

Optionally, tweak the “No archives found…” message to be generic when no time filters are provided.

---

340-343: Second occurrence of built-in any in type hint

Mirror the earlier fix to use typing.Any.

-    clp_db_connection_params: dict[str, any] = database_config.get_clp_connection_params_and_type(
+    clp_db_connection_params: dict[str, typing.Any] = database_config.get_clp_connection_params_and_type(
         True
     )

---

389-397: Protect disk deletion with try/except and fail gracefully

A failed rmtree will raise and abort the entire operation without a clear message per archive. Handle errors per archive.

-    for archive_id in archive_ids:
+    for archive_id in archive_ids:
         archive_path = archive_output_dir / archive_id
         if not archive_path.is_dir():
             logger.warning(f"Archive {archive_id} is not a directory. Skipping deletion.")
             continue
 
         logger.info(f"Deleting archive {archive_id} from disk.")
-        shutil.rmtree(archive_path)
+        try:
+            shutil.rmtree(archive_path)
+        except Exception as e:
+            logger.error(f"Failed to delete archive directory {archive_path}: {e}")
+            return -1

---

34-34: Prefer logger name name over file

Using module name is the common practice and integrates better with hierarchical logging.

-logger: logging.Logger = logging.getLogger(__file__)
+logger: logging.Logger = logging.getLogger(__name__)

components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py (3)

275-276: Type hint uses built-in any; use typing.Any

This avoids type-checker confusion.

-        clp_db_connection_params: dict[str, any] = (
+        clp_db_connection_params: dict[str, typing.Any] = (
             database_config.get_clp_connection_params_and_type(True)
         )

---

98-103: Align flag naming and consider aliasing --debug (no short -d) for compatibility

You define --verbose/-v here but the PR summary and wrapper talk about --debug; ensure consistency across modules to avoid confusion. If you want backwards compatibility without colliding with --dataset, alias the long flag only.

Apply this diff to alias --debug to the same dest (do not add -d to avoid collision with --dataset):

-    args_parser.add_argument(
-        "--verbose",
-        "-v",
-        action="store_true",
-        help="Enable debug logging.",
-    )
+    args_parser.add_argument(
+        "--verbose",
+        "--debug",
+        "-v",
+        dest="verbose",
+        action="store_true",
+        help="Enable debug logging.",
+    )

---

185-188: Fix crash: parsed_args.debug doesn’t exist; use parsed_args.verbose

This will raise AttributeError at runtime. Use the correct attribute and guard via getattr.

-    if parsed_args.debug:
+    if getattr(parsed_args, "verbose", False):
         logger.setLevel(logging.DEBUG)
     else:
         logger.setLevel(logging.INFO)

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

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.

components/clp-package-utils/clp_package_utils/scripts/archive_manager.py (1)

186-195: Log the dataset value (and when default is applied)

PR objective calls for explicitly logging the dataset used. Emit a message when defaulting and when a dataset is provided.

-    if StorageEngine.CLP_S == storage_engine:
-        dataset = CLP_DEFAULT_DATASET_NAME if dataset is None else dataset
+    if StorageEngine.CLP_S == storage_engine:
+        if dataset is None:
+            dataset = CLP_DEFAULT_DATASET_NAME
+            logger.info(f"No dataset specified; defaulting to `{dataset}`.")
+        else:
+            logger.info(f"Using dataset `{dataset}`.")
         try:
             clp_db_connection_params = clp_config.database.get_clp_connection_params_and_type(True)
             validate_dataset_name(clp_db_connection_params["table_prefix"], dataset)
         except Exception as e:
             logger.error(e)
             return -1

components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py (3)

253-260: Bug: invalid type/default for end_ts in _find_archives signature

end_ts is annotated as int but defaulted to typing.Optional[int], which is a typing object. This will break comparisons and SQL param handling. Use Optional[int] with a None default.

 def _find_archives(
     archives_dir: Path,
     database_config: Database,
     dataset: typing.Optional[str],
     begin_ts: int,
-    end_ts: int = typing.Optional[int],
+    end_ts: typing.Optional[int] = None,
 ) -> int:

---

153-157: Typo in comment: “subcomand” → “subcommand”

Minor nit for readability.

-    # Delete by ID subcomand
+    # Delete by ID subcommand

---

23-31: Avoid importing CLI constants from the wrapper; move shared constants to a common module

native.archive_manager importing from scripts.archive_manager creates cross-layer coupling. Consider placing shared constants (e.g., FIND_COMMAND, args names) in a dedicated shared module (e.g., clp_package_utils.scripts.common.archive_manager_consts) consumed by both to reduce import cycles and improve maintainability.

components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py (2)

274-279: Type hint uses built-in any; use typing.Any instead

Replace any with typing.Any for correctness with type checkers. This was flagged previously and still applies here.

-        clp_db_connection_params: dict[str, any] = (
+        clp_db_connection_params: dict[str, typing.Any] = (
             database_config.get_clp_connection_params_and_type(True)
         )

---

338-343: Type hint uses built-in any; use typing.Any instead

Same issue here; switch to typing.Any.

-    clp_db_connection_params: dict[str, any] = database_config.get_clp_connection_params_and_type(
+    clp_db_connection_params: dict[str, typing.Any] = database_config.get_clp_connection_params_and_type(
         True
     )

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 settings in your CodeRabbit configuration.

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.