Ingest async implementation #430
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # CHANGELOG.md # ingest/src/main/java/com/microsoft/azure/kusto/ingest/IngestClientBase.java
Test Results342 tests ±0 330 ✅ - 3 2m 56s ⏱️ -27s For more details on these failures, see this check. Results for commit d9d8760. ± Comparison against base commit fa305a3. This pull request removes 24 and adds 24 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
AsafMah
reviewed
Apr 20, 2025
| - [BREAKING] * Make ManagedStreamingQueuingPolicy internal, expose just a factor | ||
| * Dont allow users to pass raw data size, provide it only if we have it | ||
| - [BREAKING] Make ManagedStreamingQueuingPolicy internal, expose just a factor | ||
| - [BREAKING] Don't allow users to pass raw data size, provide it only if we have it |
There was a problem hiding this comment.
Retroactively changing the 6.0.1 changelogs? maybe another PR?
|
|
||
| ### Changed | ||
|
|
||
| - [BREAKING] All synchronous queued and streaming ingestion APIs now delegate to their asynchronous counterparts |
There was a problem hiding this comment.
Should it have any effects for the user?
I think a big thing is the exception thanges. IDK if we mention it anywhere
| * @return A configured {@link Retry} instance | ||
| */ | ||
| public Retry retry(@Nullable List<Class<? extends Throwable>> retriableErrorClasses) { | ||
| public Retry retry(@Nullable List<Class<? extends Throwable>> retriableErrorClasses, |
There was a problem hiding this comment.
Nit: the only time we use the first param is in a test. Do we really need it?
Maybe overloads to make sure they can't be used at the same time?
| * Retrieves the detailed ingestion status of | ||
| * all data ingestion operations into Kusto associated with this com.microsoft.azure.kusto.ingest.IKustoIngestionResult instance. | ||
| */ | ||
| Mono<List<IngestionStatus>> getIngestionStatusCollection() throws URISyntaxException, TableServiceErrorException; | ||
|
|
||
| int getIngestionStatusesLength(); |
| public List<IngestionStatus> getIngestionStatusCollection() { | ||
| return Collections.singletonList(this.ingestionStatus); | ||
| public Mono<List<IngestionStatus>> getIngestionStatusCollection() { | ||
| return Mono.defer(() -> { |
There was a problem hiding this comment.
I think the defer may not be needed here.
|
|
||
| ### Changed | ||
|
|
||
| - [BREAKING] All synchronous queued and streaming ingestion APIs now delegate to their asynchronous counterparts |
There was a problem hiding this comment.
Is there any public api that broke and no longer available?
|
|
||
| // If an error occurs, each time the retryWhen subscribes to executeStream create a new instance | ||
| // instead of using the same executeStream Mono for all retries | ||
| return executeStream(blobSourceInfo, ingestionProperties, blobAsyncClient, i.increment()) |
There was a problem hiding this comment.
I.increment() only happens once here, wouldn't it always be 1?
| log.error(msg, ex); | ||
| throw new IngestionClientException(msg, ex); | ||
| } | ||
| return Mono.fromCallable(() -> IngestionUtils.resultSetToStream(resultSetSourceInfo))// TODO: ? |
| ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream(); | ||
| new CsvRoutines().write(resultSetSourceInfo.getResultSet(), byteArrayOutputStream); | ||
| new CsvRoutines().write(resultSetSourceInfo.getResultSet(), byteArrayOutputStream); // TODO: CsvRoutines is not maintained from 2021. replace? |
There was a problem hiding this comment.
Maybe with something async?
| ``` | ||
|
|
||
| From File: | ||
| ```java | ||
| OperationStatus status = streamingIngestClient.ingestFromFile(fileSourceInfo, ingestionProperties).getIngestionStatusCollection().get(0).status; | ||
| OperationStatus status = streamingIngestClient.ingestFromFile(fileSourceInfo, ingestionProperties).getIngestionStatusCollection().get(0).status; //TODO: this is async now? should we have a sync equivalent? |
There was a problem hiding this comment.
Probably we want to break as least as possible
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changed
internally and block for results.
Added
enabling non-blocking operations.