feat: Add host address as scheduler and worker command line argument - #47
Conversation
WalkthroughThis pull request introduces significant changes to the Spider system's architecture by removing the automatic IP address retrieval mechanism. The modifications primarily focus on explicitly specifying host addresses through command-line arguments for schedulers and workers. The changes simplify the driver initialization process by removing the address parameter from the Changes
Possibly related PRs
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/spider/core/Driver.hpp (1)
Line range hint
13-38: Consider standardizing address handling across components.While the
Driverclass has been simplified to remove address handling, theSchedulerclass still maintains address-related fields. Consider standardizing the approach across all components for better architectural consistency.tests/integration/test_client.py (1)
22-23: Consider making the host address configurable in tests.The hardcoded "127.0.0.1" might not be suitable for all test environments. Consider:
- Using an environment variable or test configuration
- Supporting different addresses for scheduler and worker to test distributed scenarios
+# At the top of the file +SCHEDULER_HOST = os.getenv("SPIDER_TEST_SCHEDULER_HOST", "127.0.0.1") +WORKER_HOST = os.getenv("SPIDER_TEST_WORKER_HOST", "127.0.0.1") scheduler_cmds = [ str(dir_path / "spider_scheduler"), "--host", - "127.0.0.1", + SCHEDULER_HOST, "--port", str(scheduler_port), worker_cmds = [ str(dir_path / "spider_worker"), "--host", - "127.0.0.1", + WORKER_HOST,Also applies to: 32-33
tests/integration/test_scheduler_worker.py (1)
109-123: Improved test reliability with flexible state checksThe modified assertions now handle race conditions better by accepting multiple valid task states ('ready', 'running', 'success').
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/quick-start.md(2 hunks)src/spider/client/Driver.cpp(2 hunks)src/spider/core/Driver.hpp(1 hunks)src/spider/io/BoostAsio.hpp(0 hunks)src/spider/scheduler/scheduler.cpp(2 hunks)src/spider/storage/MetadataStorage.hpp(0 hunks)src/spider/storage/MysqlStorage.cpp(4 hunks)src/spider/storage/MysqlStorage.hpp(0 hunks)src/spider/worker/worker.cpp(3 hunks)tests/integration/client.py(1 hunks)tests/integration/test_client.py(2 hunks)tests/integration/test_scheduler_worker.py(5 hunks)tests/scheduler/test-SchedulerPolicy.cpp(2 hunks)tests/storage/test-DataStorage.cpp(3 hunks)tests/storage/test-MetadataStorage.cpp(1 hunks)tests/worker/test-FunctionManager.cpp(1 hunks)tests/worker/test-TaskExecutor.cpp(1 hunks)
💤 Files with no reviewable changes (3)
- src/spider/storage/MysqlStorage.hpp
- src/spider/storage/MetadataStorage.hpp
- src/spider/io/BoostAsio.hpp
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests
🔇 Additional comments (23)
src/spider/core/Driver.hpp (1)
13-13: LGTM! Constructor simplified to focus on core identity.The removal of the address parameter aligns with the PR's objective to handle IP addresses via command-line arguments instead of runtime retrieval.
src/spider/client/Driver.cpp (1)
37-37: LGTM! Consistent error handling maintained after address removal.The changes properly maintain error handling while simplifying the driver registration process.
Also applies to: 70-70
tests/integration/client.py (1)
182-182: Verify database schema changes and consider migration strategy.The removal of the address field from the drivers table requires:
- Database schema updates
- Migration strategy for existing deployments
tests/worker/test-TaskExecutor.cpp (1)
155-155: LGTM! Driver initialization simplifiedThe removal of the IP address parameter from the Driver constructor aligns with the PR's objective of eliminating runtime IP address retrieval.
tests/scheduler/test-SchedulerPolicy.cpp (1)
97-97: LGTM! Driver initialization updated correctlyThe removal of the IP address parameter from Driver constructor calls maintains consistency with the new architecture while preserving the test's functionality.
Also applies to: 144-144
tests/integration/test_scheduler_worker.py (2)
37-38: LGTM! Host address now provided via command lineThe addition of the --host argument for both scheduler and worker aligns with the PR's objective of explicitly providing IP addresses through command-line arguments.
Also applies to: 47-48
159-159: LGTM! Consistent Driver initializationThe removal of the IP address parameter from Driver initialization maintains consistency with the architectural changes.
Also applies to: 190-190
tests/worker/test-FunctionManager.cpp (1)
156-156: LGTM! Driver initialization updatedThe removal of the IP address parameter from the Driver constructor maintains consistency with the new architecture.
tests/storage/test-DataStorage.cpp (3)
28-28: LGTM! Driver instantiation updated correctly.The removal of the IP address parameter aligns with the architectural changes to simplify driver initialization.
60-60: LGTM! Consistent update to driver instantiation.The change maintains consistency with the updated Driver constructor signature.
179-180: LGTM! Driver instantiations updated correctly.Both driver instantiations have been properly updated to remove the IP address parameter.
src/spider/scheduler/scheduler.cpp (2)
45-49: LGTM! Host address option added correctly.The command-line option for host address is properly defined with clear description.
144-144: LGTM! Host argument handling implemented properly.The implementation includes:
- Proper variable declaration
- Required argument validation
- Clear error messaging
Also applies to: 152-156
src/spider/worker/worker.cpp (3)
66-66: LGTM! Host address option added correctly.The command-line option for worker host address is properly defined.
336-336: LGTM! Host argument handling implemented properly.The implementation includes proper validation and error handling for the host argument.
Also applies to: 338-351
379-379: LGTM! Driver instantiation updated correctly.The Driver constructor call has been updated to align with the new signature.
tests/storage/test-MetadataStorage.cpp (1)
34-34: LGTM! Driver instantiation updated correctly.The test case has been properly updated to use the simplified Driver constructor.
src/spider/storage/MysqlStorage.cpp (3)
64-64: LGTM! Well-designed schema change.The
addresscolumn is appropriately sized to handle both IPv4 (15 chars) and IPv6 (39 chars) addresses, with NOT NULL constraint ensuring the address is always provided.
334-335: LGTM! Clean implementation of the address management changes.The changes properly implement the transition from automatic IP address retrieval to explicit address management:
- Removed address storage from driver registration
- Added proper address and port storage for scheduler registration
Also applies to: 353-364
1491-1506: LGTM! Efficient query optimization.The method has been optimized to fetch both address and port in a single query, reducing database round trips while maintaining proper error handling and parameterization.
docs/quick-start.md (3)
149-157: LGTM! Clear and comprehensive documentation update.The scheduler setup instructions have been properly updated with:
- Clear example of the new
--hostparameter- Important production considerations regarding IP address usage
174-175: LGTM! Well-documented worker configuration.The worker setup instructions clearly document both new parameters:
- The
--hostparameter for address configuration- The
--libsparameter for task library loading
180-184: LGTM! Comprehensive notes section.The additional notes effectively cover important considerations:
- Storage URL configuration requirements
- Production IP address usage
- Task library linkage requirements
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/spider/scheduler/scheduler.cpp (2)
44-48: Enhance the help text for the host argumentThe help text could be more specific about the expected format of the host address (e.g., IPv4, IPv6, hostname).
- "scheduler host address" + "scheduler host address (e.g., '127.0.0.1', 'localhost', '::1')"
151-153: Enhance the error message for missing host argumentThe error message could be more helpful by including an example of the expected format.
- spdlog::error("host is required"); + spdlog::error("host address is required (e.g., --host \"127.0.0.1\")");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/spider/scheduler/scheduler.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: non-storage-unit-tests
- GitHub Check: lint
🔇 Additional comments (1)
src/spider/scheduler/scheduler.cpp (1)
143-155: LGTM! The implementation effectively addresses containerization requirementsThe changes successfully make the host address explicit through command-line arguments, which is crucial for containerized environments. The implementation is clean and consistent with the existing codebase style.
| if (!args.contains("host")) { | ||
| spdlog::error("host is required"); | ||
| return cCmdArgParseErr; | ||
| } | ||
| scheduler_addr = args["host"].as<std::string>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for the host address value
The code checks if the argument exists but doesn't validate the host address format or handle empty strings. Consider adding validation to ensure the address is well-formed and reachable.
scheduler_addr = args["host"].as<std::string>();
+ if (scheduler_addr.empty()) {
+ spdlog::error("host address cannot be empty");
+ return cCmdArgParseErr;
+ }
+ // Optional: Add format validation using regex or boost::asio::ip::address::from_string📝 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.
| if (!args.contains("host")) { | |
| spdlog::error("host is required"); | |
| return cCmdArgParseErr; | |
| } | |
| scheduler_addr = args["host"].as<std::string>(); | |
| if (!args.contains("host")) { | |
| spdlog::error("host is required"); | |
| return cCmdArgParseErr; | |
| } | |
| scheduler_addr = args["host"].as<std::string>(); | |
| if (scheduler_addr.empty()) { | |
| spdlog::error("host address cannot be empty"); | |
| return cCmdArgParseErr; | |
| } | |
| // Optional: Add format validation using regex or boost::asio::ip::address::from_string |
1 participant
Description
Previously, clients, schedulers and workers get their IP addresses at runtime. This approach does not work when running inside a container. Thus, now scheduler and worker gets their IP addresses as command line argument, and clients no longer need to get their addresses.
Validation performed
Summary by CodeRabbit
Release Notes
Documentation
Configuration Changes
--hostparameter for scheduler and worker startup.Database Updates
Testing
These changes streamline the Spider system's configuration and improve its network setup flexibility.