Skip to content

Feature/ingest v2#439

Open
ag-ramachandran wants to merge 50 commits intomasterfrom
feature/IngestV2
Open

Feature/ingest v2#439
ag-ramachandran wants to merge 50 commits intomasterfrom
feature/IngestV2

Conversation

@ag-ramachandran
Copy link
Contributor

Initial work on the IngestV2 API

Copy link

Copilot AI left a 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 PR introduces the initial work on the IngestV2 API, implementing a new Kotlin-based ingestion service alongside the existing Java implementation. The major change is upgrading the project from Java 8 to Java 11 and updating various dependencies to support the new ingest-v2 module.

  • Project upgraded from Java 8 to Java 11 with dependency version updates
  • New ingest-v2 module written in Kotlin with modern async capabilities
  • Centralized dependency management moved to root POM for consistency

Reviewed Changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pom.xml Upgraded Java version, updated dependencies, added ingest-v2 module, centralized dependency management
ingest/pom.xml Removed duplicate dependency management, cleaned up version specifications
data/pom.xml Removed duplicate dependency management, cleaned up version specifications
ingest-v2/pom.xml Complete new module configuration with Kotlin, Ktor, and OpenAPI generation
ingest-v2/src/main/resources/openapi.yaml OpenAPI specification for the new ingest REST API
ingest-v2/src/main/kotlin/... Core Kotlin implementation files for authentication, retry policies, data sources, and API clients
ingest-v2/src/test/kotlin/... Unit tests for retry policies and configuration API

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@github-actions
Copy link

github-actions bot commented Sep 9, 2025

Test Results

530 tests  ±0   521 ✅ ±0   8m 16s ⏱️ + 5m 13s
 31 suites ±0     9 💤 ±0 
 31 files   ±0     0 ❌ ±0 

Results for commit f61924c. ± Comparison against base commit 85e4eec.

♻️ This comment has been updated with latest results.

@AsafMah AsafMah marked this pull request as ready for review September 18, 2025 09:12
* added code for wellknown kusto endpoints
* addressed review comments
* fixed review comments

---------

Co-authored-by: ag-ramachandran <ramacg@microsoft.com>
* * Fix method signature for uploads
* * Minor edit to ConfigurationCache to determine refresh interval logic
* * Additional tests for preferred upload combinations
* * Reformat tests
* Add tests for duration
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 110 out of 116 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -0,0 +1,119 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.
package com.microsoft.azure.kusto.ingest.v2.auth.endpoints
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that we have to duplicate these entire classes.

I know we discussed before dependencies on the data package, but at this point we need to reconsider if it leaves us with this much duplicate code.


protected abstract fun self(): T

fun withAuthentication(credential: TokenCredential): T {
Copy link
Contributor

Choose a reason for hiding this comment

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

In .net, we have withNoAuthentication, here you don't, and instead throw an error if it's not called.

Either it should be part of the main ctor, or should work like c#

}

companion object {
protected fun normalizeAndCheckEngineUrl(clusterUrl: String): String {
Copy link
Contributor

Choose a reason for hiding this comment

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

These methods are wrong.

See the java getIngestionEndpoint, we have more rules there for special urls. It's also more efficient without constantly building regexes.

Another reason I think to depend on kusto-data.

* sent directly to the Data Management service.
*/
@JvmName("ingestAsync")
fun ingestAsyncJava(
Copy link
Contributor

Choose a reason for hiding this comment

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

So all of these methods are not part of the interface?

So if someone uses the interface the can't use it in java?

* Atomic counter for round-robin container selection. Increments on each
* upload to distribute load evenly across containers.
*/
private val containerIndexCounter = AtomicInteger(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm missing something, but that's not what we want -
This has a counter per uploader.

Here, if uploader A and uploader B (sharing the same configurationcache) upload, they will both use storage 0 first.

The state should be a part of the specific list inside configuration cache - so uploader A will use storage 0, and uploader B will use storage 1. See how we do it in c#

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.

4 participants