Skip to content

Conversation

@junhaoliao
Copy link
Member

@junhaoliao junhaoliao commented Oct 23, 2025

Description

This PR changes the logs directory bases back to component-specific directories before the changes to /var/log were incorrectly introduced in #1178 :

  • compression-scheduler: ./var/log -> ./var/log/compression_scheduler
  • query-scheduler: ./var/log -> ./var/log/query_scheduler

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

task

cd build/clp-package
./sbin/start-clp.sh

ls -l $(find var/log/ | grep scheduler)

->

junhao@GIGABYTE:~/workspace/2-clp/build/clp-package$ ls -l $(find var/log/ | grep scheduler)
-rw-r--r-- 1 junhao junhao   84 Oct 23 13:55 var/log/compression_scheduler/compression_scheduler.log
-rw-r--r-- 1 junhao junhao  170 Oct 23 13:55 var/log/query_scheduler/query_scheduler.log

var/log/compression_scheduler:
total 4
-rw-r--r-- 1 junhao junhao 84 Oct 23 13:55 compression_scheduler.log

var/log/query_scheduler:
total 4
-rw-r--r-- 1 junhao junhao 170 Oct 23 13:55 query_scheduler.log

Summary by CodeRabbit

Release Notes

  • Chores
    • Scheduler components now use separate, dedicated log directories for improved organization.
    • Log directories are automatically created during scheduler initialization.

@junhaoliao junhaoliao requested a review from a team as a code owner October 23, 2025 18:53
@junhaoliao
Copy link
Member Author

cc @quinntaylormitchell for awareness

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2025

Walkthrough

The changes update logging directory configurations for scheduler components. The controller module adds directory creation logic for compression and query scheduler logs. Docker compose files update environment variables to direct schedulers to their respective log subdirectories instead of the shared /var/log.

Changes

Cohort / File(s) Summary
Scheduler environment setup
components/clp-package-utils/clp_package_utils/controller.py
Added directory creation for logs in _set_up_env_for_compression_scheduler and _set_up_env_for_query_scheduler functions, ensuring the CLP logs directory exists before proceeding with environment variable setup.
Scheduler service configuration
tools/deployment/package/docker-compose.base.yaml, tools/deployment/package/docker-compose.yaml
Updated CLP_LOGS_DIR environment variables: compression-scheduler changed from "/var/log" to "/var/log/compression_scheduler"; query-scheduler changed from "/var/log" to "/var/log/query_scheduler".

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Use component-specific log directories for compression-scheduler and query-scheduler (fixes #1483)" accurately and clearly describes the main objective of the changeset. The changes across all three files—updates to both docker-compose files setting component-specific CLP_LOGS_DIR values and updates to the controller.py to create these log directories—directly align with what the title conveys. The title is specific, concise, avoids vague terminology, and uses conventional commit formatting with a "fix" prefix, making it immediately clear to reviewers what problem is being addressed.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6803e83 and 7a539e5.

📒 Files selected for processing (3)
  • components/clp-package-utils/clp_package_utils/controller.py (2 hunks)
  • tools/deployment/package/docker-compose.base.yaml (1 hunks)
  • tools/deployment/package/docker-compose.yaml (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 Learning: 2025-10-17T19:59:25.596Z
Learnt from: junhaoliao
PR: y-scope/clp#1178
File: components/clp-package-utils/clp_package_utils/controller.py:315-315
Timestamp: 2025-10-17T19:59:25.596Z
Learning: In components/clp-package-utils/clp_package_utils/controller.py, worker log directories (compression_worker, query_worker, reducer) created via `mkdir()` do not need `_chown_paths_if_root()` calls because directories are created with the same owner as the script caller. This differs from infrastructure service directories (database, queue, Redis, results cache) which do require explicit ownership changes.

Applied to files:

  • components/clp-package-utils/clp_package_utils/controller.py
📚 Learning: 2025-08-08T06:59:42.436Z
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.

Applied to files:

  • components/clp-package-utils/clp_package_utils/controller.py
🔇 Additional comments (4)
components/clp-package-utils/clp_package_utils/controller.py (2)

291-292: LGTM! Consistent with worker component pattern.

The directory creation logic for compression_scheduler follows the established pattern for worker components. Not calling _chown_paths_if_root() is correct since scheduler directories, like worker directories, inherit the script caller's ownership.

Based on learnings.


314-315: LGTM! Consistent with worker component pattern.

The directory creation logic for query_scheduler follows the established pattern for worker components. Not calling _chown_paths_if_root() is correct since scheduler directories, like worker directories, inherit the script caller's ownership.

Based on learnings.

tools/deployment/package/docker-compose.yaml (1)

41-41: LGTM! Correctly aligns with controller directory setup.

The CLP_LOGS_DIR environment variable now points to the component-specific log directory, matching the directory created by the controller at logs_directory/query_scheduler. The volume mount strategy correctly maps the host logs directory to /var/log in the container, allowing the scheduler to write to its subdirectory.

tools/deployment/package/docker-compose.base.yaml (1)

225-225: LGTM! Correctly aligns with controller directory setup.

The CLP_LOGS_DIR environment variable now points to the component-specific log directory, matching the directory created by the controller at logs_directory/compression_scheduler. The volume mount strategy correctly maps the host logs directory to /var/log in the container, allowing the scheduler to write to its subdirectory.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@junhaoliao junhaoliao merged commit 6abc934 into y-scope:main Oct 23, 2025
20 of 21 checks passed
@junhaoliao junhaoliao deleted the logs-dir branch October 23, 2025 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants