Managed streaming - better estimation for decision for streaming vs queuing #368

ohadbitt · 2024-06-30T15:06:16Z

Added

A new heuristic for choosing between queuing and streaming in Managed streaming client, the default behavior is using the
an estimation against the 4mb limit after dividing or multiplying by some factor described by the consts:

// Regardless of the format, we don't want to stream more than 10mb
int MAX_STREAMING_STREAM_SIZE_BYTES = 10 * 1024 * 1024;
// Used against the users input of raw data size
int MAX_STREAMING_RAW_SIZE_BYTES = 6 * 1024 * 1024;
double JSON_UNCOMPRESSED_FACTOR = 1.5d;
int NON_BINARY_FACTOR = 2;
double BINARY_COMPRESSED_FACTOR = 2d;
double BINARY_UNCOMPRESSED_FACTOR = 1.5d;

This will also allow users to stream bigger than 4mb non-compressed data

disableRetries option to client options - default is true only for streaming clients as if stream is not repeatable it will simply fails - better to let the user deal with it (alternative considered was to always create repeatable streams - like in managed streaming)
better error messages

github-actions · 2024-06-30T15:10:42Z

Test Results

326 tests +4 318 ✅ +4 3m 32s ⏱️ + 1m 0s
27 suites +2 8 💤 ±0
27 files +2 0 ❌ ±0

Results for commit 92efb55. ± Comparison against base commit a00bd15.

♻️ This comment has been updated with latest results.

AsafMah · 2024-07-02T10:57:32Z

This is a very big PR, can you summarize the changes in more detail?

Also, for disableRetries, maybe we want to fold this in to a more robust Policy object like @yogilad suggested and worked on.

AsafMah · 2024-07-22T08:38:03Z

ingest/pom.xml

@@ -107,6 +107,7 @@
                    <ignoredUsedUndeclaredDependencies>
                        <dependency>com.microsoft.azure:msal4j:jar</dependency>
                        <dependency>io.projectreactor:reactor-core:jar</dependency>
+                        <dependency>com.fasterxml.jackson.core:jackson-core:jar</dependency>


Why? don't we have problems with jackson in general?

It just means we get it from Data

I was sure we removed it as a dep because of blacklisting, maybe just the core part is ok? this is confusing me

im not sure we can have anything good here
but anyway Spark today releases a shaded Jar - i hope this will obsolete those problems

The issue was with one of our connectors, which blacklisted jackson. But maybe it was only jackson json and not jackson core. We'll see I guess

...src/main/java/com/microsoft/azure/kusto/data/auth/endpoints/WellKnownKustoEndpointsData.java

data/src/main/java/com/microsoft/azure/kusto/data/exceptions/ExceptionsUtils.java

data/src/main/java/com/microsoft/azure/kusto/data/http/HttpClientFactory.java

AsafMah · 2024-07-22T08:45:11Z

data/src/main/java/com/microsoft/azure/kusto/data/http/HttpClientFactory.java


+        if (properties.isDisableRetries()) {


This is in the http level, which I think is completely separate from what we do in streaming ingest.
Did you check what it actually does and when it runs? I think it does save the request, which will work for the "normal" streaming client

Well unless i missed it - it didnt - and thats why i use it here

maybe i should try and make it throw again

ingest/src/main/java/com/microsoft/azure/kusto/ingest/ManagedStreamingIngestClient.java

ingest/src/main/java/com/microsoft/azure/kusto/ingest/ManagedStreamingQueuingPolicy.java

CHANGELOG.md

AsafMah · 2024-07-30T09:34:23Z

ingest/pom.xml

@@ -107,6 +107,7 @@
                    <ignoredUsedUndeclaredDependencies>
                        <dependency>com.microsoft.azure:msal4j:jar</dependency>
                        <dependency>io.projectreactor:reactor-core:jar</dependency>
+                        <dependency>com.fasterxml.jackson.core:jackson-core:jar</dependency>


I was sure we removed it as a dep because of blacklisting, maybe just the core part is ok? this is confusing me

data/src/main/java/com/microsoft/azure/kusto/data/exceptions/ExceptionsUtils.java

ingest/src/main/java/com/microsoft/azure/kusto/ingest/source/AbstractSourceInfo.java

AsafMah · 2024-07-30T09:42:23Z

ingest/src/main/java/com/microsoft/azure/kusto/ingest/source/FileSourceInfo.java

-        this.rawSizeInBytes = rawSizeInBytes;
-    }
-
+    // An estimation of the raw (uncompressed, un-indexed) size of the data, for binary formatted files - use only if known


This is not about v2 or not v2, it's about code organization - Both StreamSourceInfo and FileSourceInfo support Compression, but in Stream it's a part of the class and for file it's calculated outside of the class.

I think it should be part of the abstract class, and FileSourceInfo can define it to be the extension or whatever.

AsafMah · 2024-07-30T09:44:19Z

ingest/src/main/java/com/microsoft/azure/kusto/ingest/IngestClientBase.java

+                    try {
+                        return ingestFromStreamImpl(streamSourceInfo,
+                                ingestionProperties);
+                    } catch (IOException e) {


Then why do you need it in the signature? Shouldn't it always be wrapped and then it's also not a breaking change

ingest/src/main/java/com/microsoft/azure/kusto/ingest/ManagedStreamingIngestClient.java

AsafMah

Basically done, just go over the rest of the comments. Most of them are nothing

AsafMah · 2024-08-28T10:26:16Z

CHANGELOG.md

@@ -4,6 +4,18 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

+## Unknown


[Unreleased] (look at the url for example)

AsafMah · 2024-08-28T10:32:10Z

ingest/pom.xml

@@ -107,6 +107,7 @@
                    <ignoredUsedUndeclaredDependencies>
                        <dependency>com.microsoft.azure:msal4j:jar</dependency>
                        <dependency>io.projectreactor:reactor-core:jar</dependency>
+                        <dependency>com.fasterxml.jackson.core:jackson-core:jar</dependency>


The issue was with one of our connectors, which blacklisted jackson. But maybe it was only jackson json and not jackson core. We'll see I guess

AsafMah · 2024-08-28T10:32:53Z

data/src/main/java/com/microsoft/azure/kusto/data/http/HttpClientFactory.java


+        if (properties.isDisableRetries()) {


AsafMah · 2024-08-28T10:34:54Z

data/src/main/java/com/microsoft/azure/kusto/data/ClientFactory.java

@@ -73,7 +75,9 @@ public static StreamingClient createStreamingClient(ConnectionStringBuilder csb)
     * @throws URISyntaxException if the cluster URL is invalid
     */
    public static StreamingClient createStreamingClient(ConnectionStringBuilder csb, HttpClientProperties properties) throws URISyntaxException {
-        return new ClientImpl(csb, properties);
+        HttpClientProperties httpClientProperties = Optional.ofNullable(properties)


That answers 1 (though I don't like it both places, but whatever)

But unlike the one from HttpClientFactory, you do disableRetries here, which I think should be commented

ingest/src/main/java/com/microsoft/azure/kusto/ingest/source/AbstractSourceInfo.java

AsafMah · 2024-08-28T11:23:15Z

ingest/src/main/java/com/microsoft/azure/kusto/ingest/IngestClientBase.java

+                    try {
+                        return ingestFromStreamImpl(streamSourceInfo,
+                                ingestionProperties);
+                    } catch (IOException e) {


I don't remember now - I think what I meant is that you can catch and throw our exception inside the impl.

EIther way it probably doesn't matter if the impl isn't public

AsafMah · 2024-08-28T11:24:40Z

ingest/src/main/java/com/microsoft/azure/kusto/ingest/ManagedStreamingIngestClient.java

    public static final String CLASS_NAME = ManagedStreamingIngestClient.class.getSimpleName();
    final QueuedIngestClientImpl queuedIngestClient;
    final StreamingIngestClient streamingIngestClient;
    private final ExponentialRetry exponentialRetryTemplate;
    private CloseableHttpClient httpClient = null;
+    private ManagedStreamingQueuingPolicy queuingPolicy = ManagedStreamingQueuingPolicy.Default;


I mean it's unlikely to grow if we're going to replace most of it for v2, but sure

AsafMah · 2024-08-28T11:28:48Z

ingest/src/main/java/com/microsoft/azure/kusto/ingest/ManagedStreamingIngestClient.java

- * If the size of the stream is bigger than {@value MAX_STREAMING_SIZE_BYTES}, it will fall back to the queued streaming client.
+ * By default the policy for choosing a queued ingestion on the first try is the checking of weather the size of the estimated
+ * raw stream size (a conversion to compressed CSV) is bigger than 4MB, it will fall back to the queued streaming client.
+ * Use SourceInfo.size to override size estimations, alternatively - use setQueuingPolicy to override the predicate heuristics.


the name is still wrong (it's SourceInfo.ge/settRawSizeInBytes)

I think just putting it right next to the previous line with this wording is confusing, since it makes it seem that they are related.

ohbitton added 5 commits March 13, 2024 23:14

stream source info size

ee61d06

some good code, unfished

91a61e4

manaed streaming queuing policy per format

06c529d

merge

d37fec4

ser raw size for stream

b8d6d3e

ohadbitt requested a review from AsafMah June 30, 2024 15:06

ohadbitt had a problem deploying to build June 30, 2024 15:06 — with GitHub Actions Failure

ohadbitt had a problem deploying to build June 30, 2024 15:06 — with GitHub Actions Error

ohbitton added 2 commits July 1, 2024 10:46

remove todo

83c1f87

format

6883b36

ohadbitt had a problem deploying to build July 1, 2024 08:45 — with GitHub Actions Failure

ohadbitt had a problem deploying to build July 1, 2024 08:45 — with GitHub Actions Error

ohadbitt had a problem deploying to build July 1, 2024 08:45 — with GitHub Actions Failure

fix test

7c9dd44

ohadbitt had a problem deploying to build July 1, 2024 13:10 — with GitHub Actions Failure

ohadbitt had a problem deploying to build July 1, 2024 13:10 — with GitHub Actions Error

ohadbitt had a problem deploying to build July 1, 2024 13:10 — with GitHub Actions Failure

print exception message on last retry

89bddb2

ohadbitt had a problem deploying to build July 2, 2024 10:42 — with GitHub Actions Failure

ohbitton added 2 commits July 2, 2024 13:57

used undeclared jackson dep

c50d56f

format

5a5ad90

ohadbitt temporarily deployed to build July 21, 2024 12:20 — with GitHub Actions Inactive

AsafMah requested changes Jul 22, 2024

View reviewed changes

AsafMah assigned ohadbitt Jul 22, 2024

good comments

f9df56d

ohadbitt had a problem deploying to build July 25, 2024 12:41 — with GitHub Actions Failure

ohbitton added 4 commits July 28, 2024 14:48

better

3c37d43

Merge remote-tracking branch 'origin/master' into streamingBetter

4c4c3e7

changelog

b6f60cc

format

4d3ae60

ohadbitt temporarily deployed to build July 29, 2024 11:15 — with GitHub Actions Inactive

AsafMah reviewed Jul 30, 2024

View reviewed changes

ohbitton added 2 commits July 30, 2024 14:05

comments - E2E

a2c4a76

changelog

2b8a0cf

ohadbitt temporarily deployed to build July 30, 2024 12:08 — with GitHub Actions Inactive

merge

92efb55

ohadbitt temporarily deployed to build August 28, 2024 10:21 — with GitHub Actions Inactive

ohadbitt deployed to build August 28, 2024 10:21 — with GitHub Actions Active

ohadbitt temporarily deployed to build August 28, 2024 10:21 — with GitHub Actions Inactive

AsafMah reviewed Aug 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Managed streaming - better estimation for decision for streaming vs queuing #368

Managed streaming - better estimation for decision for streaming vs queuing #368

ohadbitt commented Jun 30, 2024 •

edited

Loading

github-actions bot commented Jun 30, 2024 •

edited

Loading

AsafMah commented Jul 2, 2024

AsafMah Jul 22, 2024

ohadbitt Jul 23, 2024

AsafMah Jul 30, 2024

ohadbitt Jul 30, 2024

AsafMah Aug 28, 2024

AsafMah Jul 22, 2024

ohadbitt Jul 23, 2024

ohadbitt Jul 30, 2024

AsafMah Aug 28, 2024

AsafMah Jul 30, 2024

AsafMah Jul 30, 2024

AsafMah Jul 30, 2024

AsafMah left a comment

AsafMah Aug 28, 2024

AsafMah Aug 28, 2024

AsafMah Aug 28, 2024

AsafMah Aug 28, 2024

AsafMah Aug 28, 2024

AsafMah Aug 28, 2024

AsafMah Aug 28, 2024

Managed streaming - better estimation for decision for streaming vs queuing #368

Are you sure you want to change the base?

Managed streaming - better estimation for decision for streaming vs queuing #368

Conversation

ohadbitt commented Jun 30, 2024 • edited Loading

Added

github-actions bot commented Jun 30, 2024 • edited Loading

Test Results

AsafMah commented Jul 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AsafMah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadbitt commented Jun 30, 2024 •

edited

Loading

github-actions bot commented Jun 30, 2024 •

edited

Loading