-
Notifications
You must be signed in to change notification settings - Fork 44
Users/ramacg/fix review comments #457
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Users/ramacg/fix review comments #457
Conversation
…g IngestionReference and mapping cannot be passed together
…g IngestionReference and mapping cannot be passed together * Add tests for QueuedIngest with inline mapping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request refactors the ingest-v2 API to address review feedback. The main changes move database and table parameters from IngestRequestProperties to the ingestAsync methods, making IngestRequestProperties a simpler, reusable configuration object. The PR also consolidates ingestion mapping handling, removes the baseName parameter from sources (making it optional as requested), and simplifies authentication samples.
Changes:
- Moved database and table from IngestRequestProperties to ingestAsync method parameters
- Unified IngestionMapping class (replacing InlineIngestionMapping and reference-based approaches)
- Removed baseName parameter from source constructors
- Made IngestRequestProperties nullable to allow minimal configuration
- Simplified authentication in samples (QueuedIngestV2 updated, others partially)
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| IngestRequestPropertiesBuilder.kt | Removed database/table parameters from create(), unified mapping handling |
| IngestClient.kt | Added database/table parameters to ingestAsync signatures |
| StreamingIngestClient.kt | Updated to accept database/table parameters, handle nullable properties |
| QueuedIngestClient.kt | Updated signatures and null handling for properties |
| ManagedStreamingIngestClient.kt | Updated with new API signature and effective properties handling |
| IngestionMapping.kt | New unified class replacing InlineIngestionMapping |
| IngestionSource.kt | Removed baseName parameter, simplified name generation |
| Sample files | Updated to use new API, simplified authentication in QueuedIngestV2 |
| Test files | Comprehensive updates for new API, including null properties testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review comments that came up in the review session. The following key points are addressed
a) Move DB and table to ingestAsync , not in IngestRequestProperties. IRP can be completely a basic one without user having to create it everytime.
b) Validation to check if there are contradictory IngestRequestProperties (Mapping and MappingReference)
c) IngestRequestProperties can it be a C'tor or a Builder pattern?
d) Samples to use AzCli or AppId key to authenticate. Using ChainedTokenCredential may confuse users.
e) BaseName for source has to be optional. A lot of thought was put into C# to get context and debugging.