-
Notifications
You must be signed in to change notification settings - Fork 43
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
Managed streaming - better estimation for decision for streaming vs queuing #368
base: master
Are you sure you want to change the base?
Changes from all commits
ee61d06
91a61e4
06c529d
d37fec4
b8d6d3e
83c1f87
6883b36
7c9dd44
89bddb2
c50d56f
5a5ad90
b1b2e0a
f9df56d
3c37d43
4c4c3e7
b6f60cc
4d3ae60
a2c4a76
2b8a0cf
92efb55
8105207
39d1558
58bacd2
290fae3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,8 +44,7 @@ private static WellKnownKustoEndpointsData readInstance() { | |
return objectMapper.readValue(resourceAsStream, WellKnownKustoEndpointsData.class); | ||
} | ||
} catch (Exception ex) { | ||
ex.printStackTrace(); | ||
throw new RuntimeException("Failed to read WellKnownKustoEndpoints.json", ex); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you want you can annotate with @NotNull There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. who? instance? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the readInstance methods. Just so callers can know to not expect null you don't have to |
||
} | ||
return null; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
package com.microsoft.azure.kusto.data.exceptions; | ||
|
||
public class ExceptionsUtils { | ||
// Useful in IOException, where message might not propagate to the base IOException | ||
public static String getMessageEx(Exception e) { | ||
AsafMah marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return (e.getMessage() == null && e.getCause() != null) ? e.getCause().getMessage() : e.getMessage(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,6 +108,7 @@ | |
<dependency>org.reactivestreams:reactive-streams:jar</dependency> | ||
<dependency>com.microsoft.azure:msal4j:jar</dependency> | ||
<dependency>io.projectreactor:reactor-core:jar</dependency> | ||
<dependency>com.fasterxml.jackson.core:jackson-core:jar</dependency> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? don't we have problems with jackson in general? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It just means we get it from Data There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was sure we removed it as a dep because of blacklisting, maybe just the core part is ok? this is confusing me There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. im not sure we can have anything good here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
</ignoredUsedUndeclaredDependencies> | ||
<ignoreNonCompile>true</ignoreNonCompile> | ||
<ignoredNonTestScopedDependencies> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,11 @@ | ||
package com.microsoft.azure.kusto.ingest; | ||
|
||
import com.microsoft.azure.kusto.data.exceptions.ExceptionsUtils; | ||
import com.microsoft.azure.kusto.ingest.source.CompressionType; | ||
import org.apache.http.conn.util.InetAddressUtils; | ||
|
||
import java.net.InetAddress; | ||
import java.io.IOException; | ||
import java.net.URI; | ||
import java.net.UnknownHostException; | ||
import com.microsoft.azure.kusto.data.instrumentation.SupplierTwoExceptions; | ||
import com.microsoft.azure.kusto.data.instrumentation.TraceableAttributes; | ||
import com.microsoft.azure.kusto.data.instrumentation.MonitoredActivity; | ||
|
@@ -130,14 +130,20 @@ public IngestionResult ingestFromResultSet(ResultSetSourceInfo resultSetSourceIn | |
* @see IngestionProperties | ||
*/ | ||
protected abstract IngestionResult ingestFromStreamImpl(StreamSourceInfo streamSourceInfo, IngestionProperties ingestionProperties) | ||
throws IngestionClientException, IngestionServiceException; | ||
throws IngestionClientException, IngestionServiceException, IOException; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Breaking change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its not - its not public |
||
|
||
public IngestionResult ingestFromStream(StreamSourceInfo streamSourceInfo, IngestionProperties ingestionProperties) | ||
throws IngestionClientException, IngestionServiceException { | ||
// trace ingestFromStream | ||
return MonitoredActivity.invoke( | ||
(SupplierTwoExceptions<IngestionResult, IngestionClientException, IngestionServiceException>) () -> ingestFromStreamImpl(streamSourceInfo, | ||
ingestionProperties), | ||
(SupplierTwoExceptions<IngestionResult, IngestionClientException, IngestionServiceException>) () -> { | ||
try { | ||
return ingestFromStreamImpl(streamSourceInfo, | ||
ingestionProperties); | ||
} catch (IOException e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that you don't even throw IOException, in the end There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thats right - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then why do you need it in the signature? Shouldn't it always be wrapped and then it's also not a breaking change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. its in signature of the Impl .. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
throw new IngestionServiceException(ExceptionsUtils.getMessageEx(e), e); | ||
} | ||
}, | ||
getClientType().concat(".ingestFromStream")); | ||
} | ||
|
||
|
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.
So here you remove the getMessage and don't use getMessageEx - why? is it for sure not give us worse results?
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.
see my answer there