refactor: Qdrant module QOL improvements #8449
Conversation
There was a problem hiding this comment.
Thanks for your contribution, @Anush008! I've left some comments.
modules/qdrant/src/main/java/org/testcontainers/qdrant/QdrantContainer.java
Show resolved
Hide resolved
modules/qdrant/src/main/java/org/testcontainers/qdrant/QdrantContainer.java
Show resolved
Hide resolved
modules/qdrant/src/main/java/org/testcontainers/qdrant/QdrantContainer.java
Show resolved
Hide resolved
| public int getRestPort() { | ||
| return getMappedPort(QDRANT_REST_PORT); | ||
| } | ||
|
|
||
| public int getGrpcPort() { | ||
| return getMappedPort(QDRANT_GRPC_PORT); | ||
| } |
There was a problem hiding this comment.
we haven't done this in the past but I think for GRPC this makes sense? I'll discuss with the team to open QDRANT_REST_PORT and QDRANT_GRPC_PORT instead of doing it in all the modules.
modules/qdrant/src/test/java/org/testcontainers/qdrant/QdrantContainerTest.java
Outdated
Show resolved
Hide resolved
| * | ||
| * <p>Supported image: {@code qdrant/qdrant} | ||
| * | ||
| * <p>Exposed ports: | ||
| * |
There was a problem hiding this comment.
can we revert those changes, please?
| public int getHttpPort() { | ||
| return getMappedPort(QDRANT_REST_PORT); | ||
| } |
There was a problem hiding this comment.
I understand one of the ways for the GRPC client is using specifically the port but do we know of any client building the API just like GRPC?
| public int getHttpPort() { | |
| return getMappedPort(QDRANT_REST_PORT); | |
| } |
There was a problem hiding this comment.
I didn't get what you meant. Could you clarify, please?
There was a problem hiding this comment.
Is there a qdrant client that use the REST port just like GRPC does? In GRPC makes sense because the client allows it but with the REST port IDK
There was a problem hiding this comment.
It for general availability for anyone who wants to use Qdrant via the REST interface in any way.
There was a problem hiding this comment.
In that case, getHttpHostAddress() or getHttpEndpoint() make sense but since Qdrant doesn't provide a rest client, I'd prefer just to keep the getGrpcHostAddress()
String getHttpHostAddress() {
return getHost() + ":" + getMappedPort(REST_PORT);
}
String getHttpEndpoint() {
return "http://" + getHost() + ":" + getMappedPort(REST_PORT);
}| .healthCheck(QdrantOuterClass.HealthCheckRequest.getDefaultInstance()) | ||
| .get(); | ||
| QdrantClient client = new QdrantClient( | ||
| QdrantGrpcClient.newBuilder(qdrant.getHost(), qdrant.getGrpcPort(), false).build() |
There was a problem hiding this comment.
wonder why changing this test? It is also a valid way to build a client and using the getGrpcHostAddress()
There was a problem hiding this comment.
This is only a style improvement. It's comparatively simpler.
This PR makes QOL improvements to the Qdrant module.
Adds builder methods to configure an API key and config file with associated tests.