Skip to content
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

Automatic conversion of query/ingestion urls in ingest clients #268

Merged
merged 5 commits into from
Sep 13, 2022

Conversation

AsafMah
Copy link
Contributor

@AsafMah AsafMah commented Aug 18, 2022

Changes:

  • Removed complex endpoint detection system (as per discussion)
  • Automatic conversion of query/ingestion urls in ingest clients - adds or removes "ingest-"
  • Changed terminology to "ingestion endpoint" and "query endpoint" where possible
  • Deprecated un-needed ctors

@github-actions
Copy link

github-actions bot commented Aug 18, 2022

Unit Test Results

246 tests   - 1   241 ✔️  - 1   47s ⏱️ - 1m 59s
  20 suites ±0       5 💤 ±0 
  20 files   ±0       0 ±0 

Results for commit 5d05551. ± Comparison against base commit cf21875.

This pull request removes 5 and adds 4 tests. Note that renamed tests count towards both.
com.microsoft.azure.kusto.ingest.ManagedStreamingIngestClientTest ‑ CreateManagedStreamingIngestClient_WithWrongDmUri_Fail
com.microsoft.azure.kusto.ingest.ManagedStreamingIngestClientTest ‑ CreateManagedStreamingIngestClient_WithWrongEngineUri_Fail
com.microsoft.azure.kusto.ingest.QueuedIngestClientTest ‑ IngestFromFile_GivenIngestClientAndEngineEndpoint_ThrowsIngestionClientException
com.microsoft.azure.kusto.ingest.QueuedIngestClientTest ‑ IngestFromFile_GivenIngestClientAndEngineEndpoint_ThrowsIngestionServiceException
com.microsoft.azure.kusto.ingest.StreamingIngestClientTest ‑ IngestFromFile_GivenStreamingIngestClientAndDmEndpoint_ThrowsIngestionClientException
com.microsoft.azure.kusto.ingest.ManagedStreamingIngestClientTest ‑ CreateManagedStreamingIngestClient_WithDefaultCtor_WithIngestUri_Pass
com.microsoft.azure.kusto.ingest.ManagedStreamingIngestClientTest ‑ CreateManagedStreamingIngestClient_WithDefaultCtor_WithPrivateIngestUri_Pass
com.microsoft.azure.kusto.ingest.ManagedStreamingIngestClientTest ‑ CreateManagedStreamingIngestClient_WithDefaultCtor_WithPrivateQueryUri_Pass
com.microsoft.azure.kusto.ingest.ManagedStreamingIngestClientTest ‑ CreateManagedStreamingIngestClient_WithDefaultCtor_WithQueryUri_Pass

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

Merging #268 (5d05551) into master (cf21875) will decrease coverage by 0.06%.
The diff coverage is 90.00%.

@@             Coverage Diff              @@
##             master     #268      +/-   ##
============================================
- Coverage     54.42%   54.36%   -0.07%     
+ Complexity      575      565      -10     
============================================
  Files            95       95              
  Lines          3274     3232      -42     
  Branches        313      307       -6     
============================================
- Hits           1782     1757      -25     
+ Misses         1362     1353       -9     
+ Partials        130      122       -8     
Impacted Files Coverage Δ
...ure/kusto/ingest/ManagedStreamingIngestClient.java 72.41% <81.81%> (+1.50%) ⬆️
...microsoft/azure/kusto/ingest/IngestClientBase.java 87.50% <85.71%> (+32.32%) ⬆️
...rosoft/azure/kusto/ingest/IngestClientFactory.java 66.66% <100.00%> (+6.66%) ⬆️
...oft/azure/kusto/ingest/QueuedIngestClientImpl.java 72.90% <100.00%> (-0.60%) ⬇️
...soft/azure/kusto/ingest/StreamingIngestClient.java 85.71% <100.00%> (+4.17%) ⬆️
.../microsoft/azure/kusto/ingest/ResourceManager.java 73.61% <0.00%> (-3.48%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ohadbitt
Copy link
Collaborator

ohadbitt commented Aug 31, 2022

Did you test with proxies ?
If you did add a test for that

Copy link
Collaborator

@ohadbitt ohadbitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems OK

return (sourceCompressionType == null) && (dataFormat == null || dataFormat.isCompressible());
static String getQueryEndpoint(String clusterUrl) {
if (clusterUrl.contains(INGEST_PREFIX)) {
return clusterUrl.replaceFirst(INGEST_PREFIX, "");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can just do replace first - no need contains and else if

@AsafMah AsafMah merged commit e2059e7 into master Sep 13, 2022
@AsafMah AsafMah deleted the automatic-endpoint-conversion branch September 13, 2022 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants