debezium/dbz#1098 Add Qdrant connection validator#280
debezium/dbz#1098 Add Qdrant connection validator#280mfvitale merged 9 commits intodebezium:mainfrom
Conversation
.../java/io/debezium/platform/environment/connection/destination/QdrantConnectionValidator.java
Show resolved
Hide resolved
.../java/io/debezium/platform/environment/connection/destination/QdrantConnectionValidator.java
Outdated
Show resolved
Hide resolved
...r/src/test/java/io/debezium/platform/environment/connection/QdrantConnectionValidatorIT.java
Show resolved
Hide resolved
| "title": "Protocol to use (http or https)", | ||
| "default": "http" | ||
| }, | ||
| "apiKey": { |
There was a problem hiding this comment.
Currently, the apiKey property is not used in the validator implementation.
I included it in the schema for future support of Qdrant API key authentication, but since it is not being used, We can remove it to avoid confusion.
Let me know if you prefer it removed or if you would like me to add support for API key in the validator.
There was a problem hiding this comment.
If possible I'd prefer to add the support. Also if you can rename it to api.key.
@mfvitale Do you agree?
There was a problem hiding this comment.
Yeah sure no problem I will add support, I will wait for the approval.
There was a problem hiding this comment.
Yes, we should add support also for api.key
| RABBITMQ_STREAM, | ||
| RABBITMQ_NATIVE_STREAM, | ||
| MILVUS, | ||
| @JsonProperty("QDRANT") |
There was a problem hiding this comment.
Could you clarify why you added the annotation here?
There was a problem hiding this comment.
yeah sure, I added the "QDRANT" annotation for consistency with other enum values that use custom JSON names.
If it is not needed for Qdrant, I can remove it.
| @Override | ||
| public ConnectionValidationResult validate(Connection connectionConfig) { | ||
| Map<String, Object> config = connectionConfig.getConfig(); | ||
| String host = (String) config.getOrDefault("host", "localhost"); |
There was a problem hiding this comment.
Could you reason why localhost should be a good default here?
There was a problem hiding this comment.
Okay So localhost was used as a default to allow local development and testing out of the box.
I am planning to remove that.
| URL url = new URL(healthEndpoint); | ||
| HttpURLConnection conn = (HttpURLConnection) url.openConnection(); | ||
| conn.setRequestMethod("GET"); | ||
| conn.setConnectTimeout(3000); |
There was a problem hiding this comment.
Could you add a configuration property for these timeouts like in
?There was a problem hiding this comment.
yeah sure i started working on this I will refactor the validator to use a configuration property for timeouts, similar to the Kafka validator, to allow users to customize connection timeouts.
thanks for the reference!
| "protocol": { | ||
| "type": "string", | ||
| "title": "Protocol to use (http or https)", | ||
| "default": "http" |
There was a problem hiding this comment.
This is currently not supported but it is an interesting idea.
There was a problem hiding this comment.
I prefer to remove it to avoid confusion.
There was a problem hiding this comment.
yeah sure i will remove this asap.
| } | ||
|
|
||
| String apiKey = getString(config, API_KEY); | ||
| String healthEndpoint = protocol + "://" + host + ":" + port + "/healthz"; |
There was a problem hiding this comment.
WDYT to define a endpoint format string and the use "".formatted to pass the parameters?
There was a problem hiding this comment.
I think this is a great idea, I will update the validator to define health endpoint format constant and switch to .formatted(host, port) when building the URL for clarity and maintainability.
| public class QdrantConnectionValidatorIT { | ||
|
|
||
| @Test | ||
| public void testValidate_withValidConfig_shouldReturnSuccessOrFailure() { |
There was a problem hiding this comment.
Well effectively this is not a meaningful test. You shoul start a qdrant container and test the real connectivity. I suggest to have a look to what has been done with AzureEventHubsTestResource
There was a problem hiding this comment.
Thanks for the suggestion. I now replaced the previous Qdrant IT with a real Testcontainers based integration test and added a dedicated QdrantTestResource just like AzureEventHubsTestResource.
The test now validates connectivity against a running Qdrant container using the actual mapped host/port.
| URL url = new URL(healthEndpoint); | ||
| HttpURLConnection conn = (HttpURLConnection) url.openConnection(); | ||
| conn.setRequestMethod("GET"); | ||
| conn.setConnectTimeout((int) Duration.ofSeconds(defaultConnectionTimeout).toMillis()); | ||
| conn.setReadTimeout((int) Duration.ofSeconds(defaultConnectionTimeout).toMillis()); | ||
| if (!isBlank(apiKey)) { | ||
| conn.setRequestProperty("api-key", apiKey); | ||
| } | ||
| int responseCode = conn.getResponseCode(); | ||
| if (responseCode == 200) { | ||
| return ConnectionValidationResult.successful(); | ||
| } | ||
| return ConnectionValidationResult.failed("Qdrant health check failed with status: " + responseCode); |
There was a problem hiding this comment.
maybe you could wrap this into a method?
There was a problem hiding this comment.
yeah sure Thank you for the feedback @mfvitale
| } | ||
|
|
||
| @Test | ||
| public void testValidate_withValidConfig_shouldReturnSuccess() { |
There was a problem hiding this comment.
Please don't use undersocres in identifier
There was a problem hiding this comment.
yeah sure i will make changes to this and update it. thank you @jpechane
Sure, Sorry for the delay I had emergency. I was not active. I will start working. |
0e71677 to
80a1ad5
Compare
|
@gmarav05 Could you please rebase with main? |
|
Hi @gmarav05, thanks for your contribution. Please prefix the commit message(s) with the debezium/dbz#xxx GitHub issue key. |
a7cb1a5 to
039b501
Compare
| } | ||
| } | ||
|
|
||
| private static String getString(Map<String, Object> config, String key) { |
There was a problem hiding this comment.
Could you use the method that are now in ConnectionConfigUtils?
| "type": "object", | ||
| "required": [ | ||
| "host", | ||
| "hostname", |
There was a problem hiding this comment.
Ooops I will fix it asap.
There was a problem hiding this comment.
This change should not be there.
f168fd6 to
bd58189
Compare
| } | ||
|
|
||
| Integer port = ConnectionConfigUtils.getInteger(config, PORT); | ||
| if (port == null || port <= 0) { |
There was a problem hiding this comment.
@mfvitale Doesn't ignoring of negative port go against the validation prnicpile?
There was a problem hiding this comment.
Good point. Yes, we should avoid putting defaults. This is something that could be done by the UI if we want but for sure not here.
There was a problem hiding this comment.
Hello @jpechane , @mfvitale, Thank you for bringing this up. I have updated the validator to return a failure instead of defaulting to port 6333. Defaults should be handled by the UI, not the validator. by adding this return ConnectionValidationResult.failed("Port must be specified and must be a positive number for Qdrant connection"); WDYT.
Hello @jpechane. I have locally updated the validator to return a failure instead of defaulting to port 6333. Defaults should be handled by the UI, not the validator. by adding this return ConnectionValidationResult.failed("Port must be specified and must be a positive number for Qdrant connection"); Please let me know if this is correct approach. |
|
@gmarav05 Could you please rebase with latest changes on main? |
Signed-off-by: Aravind <gmarav005@gmail.com>
Signed-off-by: Aravind <gmarav005@gmail.com>
Qdrant connection validator Signed-off-by: Aravind <gmarav005@gmail.com>
Signed-off-by: Aravind <gmarav005@gmail.com>
test Signed-off-by: Aravind <gmarav005@gmail.com>
…ator Signed-off-by: Aravind <gmarav005@gmail.com>
Signed-off-by: Aravind <gmarav005@gmail.com>
…of defaulting Signed-off-by: Aravind <gmarav005@gmail.com>
b6507ac to
e5653a5
Compare
|
@gmarav05 one last this to fix and then this could be merged. |
…is host key Signed-off-by: Aravind <gmarav005@gmail.com>
|
@gmarav05 LGTM. When CI passes it can be merged |
|
@gmarav05 Applied, thanks! |
Adds Qdrant Sink Connection Validator DBZ-9441
Summary
This PR adds support for validating Qdrant Sink connections.
Changes
QdrantConnectionValidatorfor checking connectivity to Qdrant.QDRANTtype toConnectionEntity.Typeenum.connection-schemas.jsonwith Qdrant connection schema.Testing:
Fixes debezium/dbz#1098 DBZ-9441