Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
|
I would also like to add data loading support for DynamoDB to the SDK. This patch makes it easy to learn about relevant ingredients. Thank you! I am looking forward to the upcoming release. |
croud/clusters/commands.py
Outdated
| } | ||
| if args.endpoint: | ||
| extra_body["dynamodb"]["endpoint"] = args.endpoint | ||
| if args.kinesis_stream_name: |
There was a problem hiding this comment.
should there be a validation that kinesis_stream_name is required if ingestion_type is IMPORT_AND_CDC or CDC_ONLY?
There was a problem hiding this comment.
Good point, I just added a check
Co-authored-by: Thomas Achatz <44063937+tomach@users.noreply.github.com>
juanpardo
force-pushed
the
jp/dynamodb-import-support
branch
from
f501061 to
10571bb
Compare
croud/clusters/commands.py
Outdated
| extra_body["ingestion_type"] = args.ingestion_type | ||
| if "CDC" in extra_body.get("ingestion_type", ""): | ||
| if not args.kinesis_stream_name: | ||
| raise Exception( |
There was a problem hiding this comment.
Raising a plain Exception here will print a full stack trace I think? maybe raise a ValueError so the error looks a bit better? sth like
raise ValueError(
"--kinesis-stream-name must be set when using CDC ingestion types."
)
There was a problem hiding this comment.
Raising any exception leads to a stack trace, so I had to end up using a controlled error exit
| ``clusters import-jobs create from-dynamodb`` | ||
| ============================================= | ||
|
|
||
| .. argparse:: | ||
| :module: croud.__main__ | ||
| :func: get_parser | ||
| :prog: croud | ||
| :path: clusters import-jobs create from-dynamodb | ||
|
|
||
| .. code-block:: console |
There was a problem hiding this comment.
You need to change the titles level.
I've also added a note about the waiting to finish like the others, but actually I don't know how it behave. I let you fix the text I suggested if it is not accurate.
| ``clusters import-jobs create from-dynamodb`` | |
| ============================================= | |
| .. argparse:: | |
| :module: croud.__main__ | |
| :func: get_parser | |
| :prog: croud | |
| :path: clusters import-jobs create from-dynamodb | |
| .. code-block:: console | |
| ``clusters import-jobs create from-dynamodb`` | |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |
| .. note:: | |
| For IMPORT_ONLY, this command will wait for the operation to finish or fail. | |
| .. argparse:: | |
| :module: croud.__main__ | |
| :func: get_parser | |
| :prog: croud | |
| :path: clusters import-jobs create from-dynamodb | |
| Example | |
| ^^^^^^^ | |
| .. code-block:: console |
There was a problem hiding this comment.
Fixed this, thanks!
|
|
||
| For IMPORT_ONLY, this command will wait for the operation to finish or fail. | ||
| When --ingestion-type is set to CDC_ONLY or IMPORT_AND_CDC, the command will not finish | ||
| and even when the last CDC event is processed, it will remain waiting for new CDC events to come. |
There was a problem hiding this comment.
I'm wondering if we should change a bit the command behavior when it's CDC (and for MongoDB as well) to make it just start the importJob and not wait at all.
There was a problem hiding this comment.
Would you wait for the import part to finish in an IMPORT_AND_CDC job though? Happy to discuss what the best behaviour would be for MongoDB and DynamoDB, but perhaps we can do it in a separate issue :)
There was a problem hiding this comment.
You're right, it can be done in a separate issue. Will you create it?
4 participants
Summary of the changes / Why this is an improvement
Added support for DynamoDB data jobs
Checklist