feat(package): Enable replica set for the MongoDB results cache and configure it when starting the package. #632
Conversation
…en starting the package.
|
Warning Rate limit exceeded@junhaoliao has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces modifications to support MongoDB replica set initialization in a Python utility script and updates the MongoDB configuration file. The changes enable automatic replica set configuration during the index creation process for a stream collection. The script now checks the replica set status and initializes it if not already configured, ensuring proper MongoDB replication setup. Changes
Sequence DiagramsequenceDiagram
participant Script as Create Results Cache Indices Script
participant MongoDB as MongoDB Client
Script->>MongoDB: Establish Connection
Script->>MongoDB: Check Replica Set Status
alt Replica Set Not Initialized
Script->>MongoDB: Initialize Replica Set
MongoDB-->>Script: Replica Set Configured
else Replica Set Already Initialized
Script-->>MongoDB: Continue Processing
end
Script->>MongoDB: Create Indexes
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 (
|
|
An issue is found when in However, such issue is only observed on By looking into the community Mongo server source code, it was found they have two ways to check whether a node is up before the server configures itself as a replica node:
Why
|
|
You may also see from the |
junhaoliao
changed the title
junhaoliao
changed the title
junhaoliao
changed the title
![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 (2)
components/clp-py-utils/clp_py_utils/create-results-cache-indices.py (2)
19-33: Consider parametrising the replica set host and port.Although this initialisation flow works well for the default port (27017) on localhost, it may not be sufficient if the user wants to set up a replica set on a non-standard port or host. Passing in the host and port as parameters will improve flexibility and will align with the PR’s objective of making replica set configuration more adaptable.
47-49: Avoid creating two separate MongoClient connections.Re-using the same
MongoClientafter invokinginitialize_replica_setcan be more efficient and simpler. If you do require a new client connection for index creation, consider adding a short comment explaining why a fresh connection is needed, so it is clear to maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/clp-py-utils/clp_py_utils/create-results-cache-indices.py(3 hunks)components/package-template/src/etc/mongo/mongod.conf(1 hunks)
🔇 Additional comments (2)
components/clp-py-utils/clp_py_utils/create-results-cache-indices.py (1)
6-6: Good use of the OperationFailure import.
This import from pymongo.errors is essential for handling replica set status checks and ensures that any non-initialised replica set is properly caught and configured.
components/package-template/src/etc/mongo/mongod.conf (1)
3-4: Replica set configuration approved.
Enabling the replica set by specifying replSetName is a straightforward and effective strategy. If you later find a need to run multiple replica sets on the same machine, you may consider making rs0 a configurable parameter in the environment or in a generated config file.
|
Quick question: for Result cache host switching validation, I assumed you also ensured the search in webui is still working with different IPs and ports? |
|
Discussed with Junhao offline and clarified the two points:
Imo, we should make these two points clear in the code. We have agreed on renaming some replica related functions and adding docstrings so developer who only reads code can also have the context above. |
| @@ -491,6 +491,7 @@ def start_results_cache(instance_id: str, clp_config: CLPConfig, conf_dir: pathl | |||
| cmd = [ | |||
| "docker", "run", | |||
| "-d", | |||
| "--network", "host", | |||
There was a problem hiding this comment.
Just curious, do you know exactly why we need --network host? is there any IP other than results_cache.port that will be accessed?
There was a problem hiding this comment.
Using --network host with MongoDB in Docker ensures proper replica set configuration because Docker's network containerization interferes with MongoDB's isSelf checks when using non-localhost-&-27017 ports. By exposing the host network to the MongoDB container, we can avoid this issue.
More details of the investigation can be found at #632 (comment) Let me know if I can elaborate on any part
There was a problem hiding this comment.
shall we also consider renaming this script as initialize-results-cache.py to better reflect its updated purpose?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/clp-py-utils/clp_py_utils/create-results-cache-indices.py (3)
1-1: Consider renaming the script to better reflect its purpose.As suggested in the previous review, renaming this script to
initialize-results-cache.pywould better represent its expanded functionality, which now includes both index creation and replica set initialization.
20-48: Well-structured replica set status checking with comprehensive error handling.The function properly handles different MongoDB error codes and provides appropriate logging for each case:
- Code 94: Not yet initialized
- Code 93: Invalid replica set configuration
- Other codes: Re-raised for proper error propagation
Consider adding docstring documentation:
def check_replica_set_status(client, netloc): + """ + Check the status of MongoDB replica set. + + Args: + client: MongoDB client instance + netloc: Network location string (host:port) + + Returns: + tuple: (is_already_initialized, should_configure_replica_set) + + Raises: + OperationFailure: For unexpected MongoDB operation errors + """
51-69: Robust replica set configuration implementation.The function handles both initial configuration and reconfiguration scenarios appropriately. The use of
force=Truefor reconfiguration is a good choice to avoid version management complexity.Consider adding a warning log before force reconfiguration:
if is_already_initialized: + logger.warning("Force reconfiguring existing replica set") # Use `force=True` so that we do not have to increment the version client.admin.command("replSetReconfig", config, force=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py(2 hunks)components/clp-py-utils/clp_py_utils/create-results-cache-indices.py(3 hunks)components/package-template/src/etc/mongo/mongod.conf(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (4)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (2)
496-496: Network configuration change aligns with MongoDB replica set requirements.The addition of
--network hostis necessary for proper replica set initialization in Docker, as explained in the previous discussion. This allows MongoDB to perform itsisSelfchecks correctly.
508-511: Direct port and IP binding replaces Docker port mapping.The change from Docker port mapping to direct MongoDB configuration using
--bind_ipand--portis more suitable for replica set configuration. This approach:
- Avoids network translation issues that could interfere with replica set communication
- Provides clearer network configuration that matches the replica set configuration
components/package-template/src/etc/mongo/mongod.conf (1)
1-2: Replica set configuration is properly defined.The configuration correctly sets up the replica set name as "rs0", which matches the configuration used in the Python initialization code.
Let's verify that this replica set name is consistently used across the codebase:
✅ Verification successful
Replica set configuration is consistent across the codebase
The replica set name "rs0" is correctly configured in mongod.conf and properly referenced in the initialization script. The configuration follows MongoDB best practices with explicit host specifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for "rs0" references to ensure consistency rg "rs0" --type yaml --type pythonLength of output: 65
Script:
#!/bin/bash # Search for "rs0" across all files rg "rs0" # Find MongoDB-related Python files fd -e py -e yaml -e conf -e config | rg -i "mongo"Length of output: 279
Script:
#!/bin/bash # Check the context of rs0 usage in the indices script rg -A 5 -B 5 "rs0" components/clp-py-utils/clp_py_utils/create-results-cache-indices.pyLength of output: 462
components/clp-py-utils/clp_py_utils/create-results-cache-indices.py (1)
97-98: Ensure replica set is configured before creating indices.The code correctly uses
directConnection=Truefor the initial connection to avoid replica set connection attempts before configuration. However, there's a potential race condition between the two client connections.Consider adding a retry mechanism for the second connection:
with MongoClient(results_cache_uri, directConnection=True) as results_cache_client: configure_replica_set_if_needed(results_cache_client, results_cache_uri) + + # Wait for replica set to become available + max_retries = 30 + retry_interval = 1 + for attempt in range(max_retries): + try: + with MongoClient(results_cache_uri, serverSelectionTimeoutMS=1000) as client: + client.admin.command('ping') + break + except Exception as e: + if attempt == max_retries - 1: + raise + time.sleep(retry_interval)✅ Verification successful
Add retry mechanism for MongoDB replica set initialization
The review comment is correct. After configuring a replica set, MongoDB needs time to elect a primary and stabilize. The suggested retry mechanism will help handle the transition period properly and prevent connection failures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for configure_replica_set_if_needed implementation ast-grep --pattern 'def configure_replica_set_if_needed($$$):' # Look for MongoDB connection patterns and retry mechanisms rg -A 10 "MongoClient\(" --type py # Search for retry-related patterns rg "retry|attempt|max_retries" --type pyLength of output: 6544
Script:
#!/bin/bash # Search for configure_replica_set_if_needed implementation with content rg -A 10 "configure_replica_set_if_needed" --type pyLength of output: 2583
…improve readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/clp-py-utils/clp_py_utils/create-results-cache-indices.py (3)
20-64: Consider adding more detailed logging for replica set status changes.The logging could be more consistent. Consider adding INFO level logs for important state changes, while keeping DEBUG level for detailed operations.
- logger.debug("Replica set is already initialized at %s", existing_netloc) + logger.info("Replica set is already initialized at %s", existing_netloc)
94-115: Add validation for MongoDB URI components.The URI validation could be more thorough to prevent issues with invalid configurations.
parsed_uri = urlparse(uri) netloc = parsed_uri.netloc - if 0 == len(netloc): + if not netloc: raise ValueError("Invalid URI: %s", uri) + + # Validate URI components + if not parsed_uri.hostname: + raise ValueError("Missing hostname in URI: %s" % uri) + if not parsed_uri.port: + raise ValueError("Missing port in URI: %s" % uri)
129-131: Consider consolidating MongoDB connections and improving error handling.The code uses two separate client connections: one for replica set initialization and another for index creation. This could be confusing and potentially inefficient.
- with MongoClient(results_cache_uri, directConnection=True) as results_cache_client: - init_replica_set_for_oplog_if_needed(results_cache_client, results_cache_uri) - - with MongoClient(results_cache_uri) as results_cache_client: + # Initialize with direct connection for replica set setup + results_cache_client = MongoClient(results_cache_uri, directConnection=True) + try: + init_replica_set_for_oplog_if_needed(results_cache_client, results_cache_uri) + # Reconnect to use replica set + results_cache_client = MongoClient(results_cache_uri) stream_collection = results_cache_client.get_default_database()[stream_collection_name] + except OperationFailure as e: + logger.error("Failed to initialize replica set: %s", str(e)) + raise + finally: + results_cache_client.close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/clp-py-utils/clp_py_utils/create-results-cache-indices.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (3)
components/clp-py-utils/clp_py_utils/create-results-cache-indices.py (3)
4-7: LGTM! The new imports support the replica set functionality.The added imports are appropriate for URI parsing and MongoDB error handling.
1-1: Consider renaming the script to better reflect its purpose.As suggested in previous reviews, the script now handles both index creation and replica set initialization. Consider renaming it to
initialize-results-cache.pyto better reflect its expanded purpose.
66-92: 🛠️ Refactor suggestionAdd safety checks and clarify single-node nature in function name.
- The function name doesn't indicate it's specifically for single-node setup
- Using force=True in replSetReconfig could be dangerous without additional checks
-def init_replica_set_for_oplog(client: MongoClient, netloc: str, force: bool): +def init_single_node_replica_set(client: MongoClient, netloc: str, force: bool): """ - Initializes or reconfigures a single-node MongoDB replica set for enabling the oplog. + Initializes or reconfigures a single-node MongoDB replica set. + + WARNING: This function is specifically designed for single-node replica sets. + Using force=True in reconfiguration can be dangerous in multi-node setups. :param client: :param netloc: The network location of the MongoDB instance to configure as a replica set member. :param force: Whether this is a reconfiguration operation. """ + if not netloc: + raise ValueError("netloc cannot be empty")Likely invalid or redundant comment.
|
@haiqi96 I added docstrings for the newly added functions in the init script, and renamed the methods & parameters & the init script name, which hopefully has clarified the purpose of those methods. Please help taking a look again. |
components/clp-py-utils/clp_py_utils/initialize-results-cache.py
Outdated
Show resolved
Hide resolved
| def check_replica_set_status(client: MongoClient, netloc: str) -> tuple[bool, bool]: | ||
| """ | ||
| Checks the current replica set status of the MongoDB server and determines whether it needs to | ||
| be configured or reconfigured. |
components/clp-py-utils/clp_py_utils/initialize-results-cache.py
Outdated
Show resolved
Hide resolved
components/clp-py-utils/clp_py_utils/initialize-results-cache.py
Outdated
Show resolved
Hide resolved
Co-authored-by: haiqi96 <14502009+haiqi96@users.noreply.github.com>
components/clp-py-utils/clp_py_utils/initialize-results-cache.py
Outdated
Show resolved
Hide resolved
Description
hostand specify--bind-ipand--portwhen starting the mongo image.Validation performed
Search query performance compassion
With & Without the replica set enabled, performed a search query in the WebUI with a screen recording of 30fps, and extract below times:
The speed performance does not seem to significantly differ with and without replica-set enabled.
Result cache host switching
Previous attempts to enable replica set failed because there were host reachability issues observed. Below validations ensure the
clp-config.yml/results_cache host and port are configurable without affecting the result cache's avaialbility.build/clp-package/lib/python3/site-packages/clp_py_utils/create-results-cache-indices.pyBuilt the clp-package.
clp-package/sbin/start-clp.shwith the default config and observed below logs:Changed the
result_cache'shostandportconfig inclp-package/etc/clp-config.ymlto belocalhost:27018. e.g.,clp-package/sbin/start-clp.shand observed below logs:Used
ip addrto find another address that belongs to the clp-package host (i got172.22.78.194). Changed theresult_cache'shostandportconfig inclp-package/etc/clp-config.ymlto be172.22.78.194:27018.clp-package/sbin/start-clp.shand observed below logs:Changed the
result_cache'shostandportconfig inclp-package/etc/clp-config.ymlto belocalhost:27017.clp-package/sbin/start-clp.shand observed below logs:Summary by CodeRabbit
Configuration Changes
MongoDB Management
Deployment Updates