Fix UAV migration bug: look up editableMappingsInfo versions in correct list #8378
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new script that inserts editable mapping information into a FossilDB database and updates the migration logic in an existing script. The new Python script uses command-line arguments, logging, and a gRPC call to perform database insertions. Additionally, the migration method now checks the correct mapping identifier when verifying the existence of a materialized version, with a reordering of the byte retrieval process. Changes
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tools/migration-unified-annotation-versioning/add_editable_mapping_info.py(1 hunks)tools/migration-unified-annotation-versioning/migration.py(1 hunks)
🔇 Additional comments (3)
tools/migration-unified-annotation-versioning/add_editable_mapping_info.py (2)
1-10: LGTM! Clean and well-organized imports.The imports are logically grouped and include all necessary dependencies for logging, argument parsing, and FossilDB communication.
35-36: LGTM! Standard Python entry point pattern.The entry point follows the standard Python pattern for script execution.
tools/migration-unified-annotation-versioning/migration.py (1)
460-462: LGTM! Bug fix for version lookup.The fix correctly uses
mapping_idinstead oftracing_idwhen checking for materialized versions in the layer version mapping. This aligns with the PR objectives to fix the UAV migration bug where editableMappingInfo versions were being looked up in the wrong list.Let's verify that this is the only place where we need to make this change:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other potential places where we might need to make similar changes ast-grep --pattern 'layer_version_mapping[$_][$_]'Length of output: 675
Verification of Version Lookup Fix in Editable Mapping Info
The fix in the
migrate_editable_mapping_infofunction (lines 460–463) correctly switches to usingmapping_idfor version lookup. Our search shows that the other usages oflayer_version_mapping(in lines 319, 345, and 427) still usetracing_idortracing_or_mapping_idin different contexts. This indicates that the change is isolated to editable mapping info where the correct key is indeedmapping_id, and the other occurrences remain intentional.
| def main(): | ||
| logger.info("Hello from add_editable_mapping_info") | ||
| setup_logging() | ||
| parser = argparse.ArgumentParser() | ||
| parser.add_argument("--fossil", type=str, help="Fossildb host and port. Example: localhost:7155", required=True) | ||
| parser.add_argument("--editable_mapping_info_file", type=str, help="path to binary input file to put", required=True) | ||
| parser.add_argument("--tracingId", type=str, help="tracingId to put the entry to", required=True) | ||
| parser.add_argument("--version", type=int, help="version number to put the entry to", required=True) | ||
| args = parser.parse_args() | ||
| before = time.time() | ||
| stub = connect_to_fossildb(args.fossil, "target") | ||
| with open(args.editable_mapping_info_file, 'rb') as infile: | ||
| bytes_to_put = infile.read() | ||
|
|
||
| logger.info(f"putting {len(bytes_to_put)} bytes of type {type(bytes_to_put)} at {args.tracingId} v{args.version}") | ||
| reply = stub.Put(proto.PutRequest(collection="editableMappingsInfo", key=args.tracingId, version=args.version, value=bytes_to_put)) | ||
| assert_grpc_success(reply) | ||
| log_since(before, "Inserting one editable mapping info entry") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file operations and gRPC calls.
The script needs better error handling for:
- File operations when reading the binary input file
- gRPC connection failures
- Invalid input validation
Apply this diff to add error handling:
def main():
logger.info("Hello from add_editable_mapping_info")
setup_logging()
parser = argparse.ArgumentParser()
parser.add_argument("--fossil", type=str, help="Fossildb host and port. Example: localhost:7155", required=True)
parser.add_argument("--editable_mapping_info_file", type=str, help="path to binary input file to put", required=True)
parser.add_argument("--tracingId", type=str, help="tracingId to put the entry to", required=True)
parser.add_argument("--version", type=int, help="version number to put the entry to", required=True)
args = parser.parse_args()
before = time.time()
- stub = connect_to_fossildb(args.fossil, "target")
- with open(args.editable_mapping_info_file, 'rb') as infile:
- bytes_to_put = infile.read()
+ try:
+ if args.version < 0:
+ raise ValueError("Version number must be non-negative")
+
+ stub = connect_to_fossildb(args.fossil, "target")
+
+ try:
+ with open(args.editable_mapping_info_file, 'rb') as infile:
+ bytes_to_put = infile.read()
+ except IOError as e:
+ logger.error(f"Failed to read input file: {e}")
+ raise
- logger.info(f"putting {len(bytes_to_put)} bytes of type {type(bytes_to_put)} at {args.tracingId} v{args.version}")
- reply = stub.Put(proto.PutRequest(collection="editableMappingsInfo", key=args.tracingId, version=args.version, value=bytes_to_put))
- assert_grpc_success(reply)
- log_since(before, "Inserting one editable mapping info entry")
+ if not bytes_to_put:
+ raise ValueError("Input file is empty")
+
+ logger.info(f"putting {len(bytes_to_put)} bytes of type {type(bytes_to_put)} at {args.tracingId} v{args.version}")
+ try:
+ reply = stub.Put(proto.PutRequest(collection="editableMappingsInfo", key=args.tracingId, version=args.version, value=bytes_to_put))
+ assert_grpc_success(reply)
+ log_since(before, "Inserting one editable mapping info entry")
+ except Exception as e:
+ logger.error(f"Failed to put data to FossilDB: {e}")
+ raise
+ except Exception as e:
+ logger.error(f"Script failed: {e}")
+ sys.exit(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 main(): | |
| logger.info("Hello from add_editable_mapping_info") | |
| setup_logging() | |
| parser = argparse.ArgumentParser() | |
| parser.add_argument("--fossil", type=str, help="Fossildb host and port. Example: localhost:7155", required=True) | |
| parser.add_argument("--editable_mapping_info_file", type=str, help="path to binary input file to put", required=True) | |
| parser.add_argument("--tracingId", type=str, help="tracingId to put the entry to", required=True) | |
| parser.add_argument("--version", type=int, help="version number to put the entry to", required=True) | |
| args = parser.parse_args() | |
| before = time.time() | |
| stub = connect_to_fossildb(args.fossil, "target") | |
| with open(args.editable_mapping_info_file, 'rb') as infile: | |
| bytes_to_put = infile.read() | |
| logger.info(f"putting {len(bytes_to_put)} bytes of type {type(bytes_to_put)} at {args.tracingId} v{args.version}") | |
| reply = stub.Put(proto.PutRequest(collection="editableMappingsInfo", key=args.tracingId, version=args.version, value=bytes_to_put)) | |
| assert_grpc_success(reply) | |
| log_since(before, "Inserting one editable mapping info entry") | |
| def main(): | |
| logger.info("Hello from add_editable_mapping_info") | |
| setup_logging() | |
| parser = argparse.ArgumentParser() | |
| parser.add_argument("--fossil", type=str, help="Fossildb host and port. Example: localhost:7155", required=True) | |
| parser.add_argument("--editable_mapping_info_file", type=str, help="path to binary input file to put", required=True) | |
| parser.add_argument("--tracingId", type=str, help="tracingId to put the entry to", required=True) | |
| parser.add_argument("--version", type=int, help="version number to put the entry to", required=True) | |
| args = parser.parse_args() | |
| before = time.time() | |
| try: | |
| if args.version < 0: | |
| raise ValueError("Version number must be non-negative") | |
| stub = connect_to_fossildb(args.fossil, "target") | |
| try: | |
| with open(args.editable_mapping_info_file, 'rb') as infile: | |
| bytes_to_put = infile.read() | |
| except IOError as e: | |
| logger.error(f"Failed to read input file: {e}") | |
| raise | |
| if not bytes_to_put: | |
| raise ValueError("Input file is empty") | |
| logger.info(f"putting {len(bytes_to_put)} bytes of type {type(bytes_to_put)} at {args.tracingId} v{args.version}") | |
| try: | |
| reply = stub.Put(proto.PutRequest(collection="editableMappingsInfo", key=args.tracingId, version=args.version, value=bytes_to_put)) | |
| assert_grpc_success(reply) | |
| log_since(before, "Inserting one editable mapping info entry") | |
| except Exception as e: | |
| logger.error(f"Failed to put data to FossilDB: {e}") | |
| raise | |
| except Exception as e: | |
| logger.error(f"Script failed: {e}") | |
| sys.exit(1) |
Follow-up for #7917 – a bug in the migration led to some versions of the editableMappingInfo being skipped. We repaired this for the first migrated instance using the added python script.