Skip to content

first draft of Bulk API and retrieve tests #122

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

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Conversation

enikao
Copy link
Contributor

@enikao enikao commented Jan 4, 2024

No description provided.

@dslmeinte
Copy link
Contributor

Good stuff, nice progress! How could we keep this aligned (to a sensible degree) with the TypeScript implementation?

@enikao
Copy link
Contributor Author

enikao commented Jan 4, 2024

Good stuff, nice progress! How could we keep this aligned (to a sensible degree) with the TypeScript implementation?

By running the tests against it

@joswarmer
Copy link

joswarmer commented Jan 4, 2024

Nice! We should be able to run this client against the typescript-repository as well. Just need to make sure the calls are exactly the same.
Also, the response expected is not identical to the response in the typescript repository, but we should be able to unify these.

@ftomassetti
Copy link
Contributor

I played with the idea of making the code in https://github.com/Strumenta/starlasu-lionweb-repository-client implement the interfaces exposed by this PR. The first issue I ran into is that the code in my client is asynchronous (I used Kotlin's coroutines) while this code is synchronous. I can make the asynchronous code synchronous, like this:

    // this is what I had before, notice the suspend keyword. I think it similar to state that the 
    // method return Promise<List<String>>
    suspend fun getPartitionIDs(): List<String> {
        val response: HttpResponse = client.get("http://$hostname:$port/bulk/partitions")
        val data = response.bodyAsText()
        val chunk = LowLevelJsonSerialization().deserializeSerializationBlock(data)
        return chunk.classifierInstances.mapNotNull { it.id }
    }

    // This is how I could adapt the code to implement the IBulkLowLevel interface
    override fun partitions(): IPartitionsResponse {
        // basically I wrap the same code as before in a runBlocking and add some code to
        // build the IPartitionsResponse instance
        return runBlocking {
            val response: HttpResponse = client.get("http://$hostname:$port/bulk/partitions")
            val data = response.bodyAsText()
            val chunk = LowLevelJsonSerialization().deserializeSerializationBlock(data)
            object : IPartitionsResponse {
                override fun isOk(): Boolean = true

                override fun getErrorMessage(): String {
                    TODO("Not yet implemented")
                }

                override fun getResult(): SerializedChunk {
                    return chunk
                }

            }
        }
    }

But this raised the question: would it make sense to have the interfaces implement asynchronous methods and then have a wrapper that makes those calls synchronous for clients who prefer synchronous APIs?
My logic is that asynchronous interfaces are more generic, as they can be made synchronous, while synchronous interfaces cannot be made asynchronous. Does this make sense?
While in Java there are no coroutines one could use promises, futures, or callbacks (not really an expert in async programming).

@dslmeinte
Copy link
Contributor

But this raised the question: would it make sense to have the interfaces implement asynchronous methods and then have a wrapper that makes those calls synchronous for clients who prefer synchronous APIs?

Maybe the other way around: define a synchronous API, and (re-)wrap that in an asynchronous one, and always use and implement the latter.

My logic is that asynchronous interfaces are more generic, as they can be made synchronous, while synchronous interfaces cannot be made asynchronous. Does this make sense? While in Java there are no coroutines one could use promises, futures, or callbacks (not really an expert in async programming).

Uhm, synchronous can be made asynchronous but not the other way around. (“Asynchronicity is viral.”) I think it's an inherent property of distributed systems that calls are inherently asynchronous.

@enikao
Copy link
Contributor Author

enikao commented Jan 8, 2024

I agree with Meinte, the code should stay synchronous. Java allows wrapping in both directions, but the API is cleaner in the sync variant, and it is a lot easier to debug.

@ftomassetti
Copy link
Contributor

I agree with Meinte, the code should stay synchronous. Java allows wrapping in both directions, but the API is cleaner in the sync variant, and it is a lot easier to debug.

Ok then, thank you for the comments

@ftomassetti
Copy link
Contributor

Eventually we took inspiration from this and introduced a synchronous interface (BulkAPIClient). In this PR there are however also tests. Should we aim to use them or should we close this PR?

@enikao
Copy link
Contributor Author

enikao commented Aug 12, 2025

Eventually we took inspiration from this and introduced a synchronous interface (BulkAPIClient). In this PR there are however also tests. Should we aim to use them or should we close this PR?

I guess they should end up in the integration repo -- we should have something similar as the delta tests also for bulk.
So shall we create an issue there to take over the examples from this PR?

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