Conversation
...service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchClient.java
Show resolved
Hide resolved
...adata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchClient.java
Show resolved
Hide resolved
| RestClientBuilder builder = | ||
| RestClient.builder(HttpHost.create(ELASTIC_SEARCH_CONTAINER.getHttpHostAddress())) | ||
| String hostAddress = ELASTIC_SEARCH_CONTAINER.getHttpHostAddress(); | ||
| String[] parts = hostAddress.split(":"); |
There was a problem hiding this comment.
⚠️ Edge Case: Host address parsing fails if address contains IPv6 or scheme
In multiple test files, getHttpHostAddress() return value is split naively on : to extract host and port:
String hostAddress = ELASTIC_SEARCH_CONTAINER.getHttpHostAddress();
String[] parts = hostAddress.split(":");
HttpHost httpHost = new HttpHost("http", parts[0], Integer.parseInt(parts[1]));This parsing will break if:
getHttpHostAddress()returns a value containing a scheme prefix (e.g.,http://localhost:9200) —parts[0]would behttpandparts[1]would be//localhost- The host is an IPv6 address (e.g.,
[::1]:9200)
While Testcontainers typically returns host:port format, this is fragile. A safer approach would be to use URI parsing:
String hostAddress = ELASTIC_SEARCH_CONTAINER.getHttpHostAddress();
URI uri = URI.create("http://" + hostAddress);
HttpHost httpHost = new HttpHost("http", uri.getHost(), uri.getPort());Or use HttpHost.create("http://" + hostAddress) if available in HC5.
Was this helpful? React with 👍 / 👎
| os.org.opensearch.client.Response response = lowLevelClient.performRequest(request); | ||
| } else if (searchClient instanceof OpenSearchClient openSearchClient) { | ||
| os.org.opensearch.client.opensearch.generic.OpenSearchGenericClient genericClient = | ||
| openSearchClient.getNewClient().generic(); |
There was a problem hiding this comment.
⚠️ Bug: NPE risk: getNewClient() may return null in OpenMetadataOperations
In OpenMetadataOperations.java line 2012, openSearchClient.getNewClient().generic() calls .generic() directly on the result of getNewClient(). However, OpenSearchClient.newClient is set by createOpenSearchNewClient(transport) which returns null on failure (line 136 of OpenSearchClient.java). The Lombok @Getter exposes this as getNewClient().
If the client initialization failed for any reason, this would throw a NullPointerException.
Suggested fix: Add a null check before using:
} else if (searchClient instanceof OpenSearchClient openSearchClient) {
var osClient = openSearchClient.getNewClient();
if (osClient == null) {
LOG.error("OpenSearch client not available. Cannot retrieve indices.");
return indices;
}
OpenSearchGenericClient genericClient = osClient.generic();
// ...
}Alternatively, use the existing isClientAvailable check.
Was this helpful? React with 👍 / 👎
🔍 CI failure analysis for aefe33b: Eight failures: infrastructure (validate-docker-compose); external repo (maven-collate-ci); UI tests (playwright); database constraints (integration-tests); Python tests (no Python modified); backend unit tests have 5 of 7,918 errors (3 AWS credential tests, 2 workflow timeouts) - 99.94% pass rate.IssueEight CI failures detected:
Root Causevalidate-docker-compose FailureInfrastructure issue - snapshot Docker images aren't published to the registry (persistent known issue). maven-collate-ci FailureThe external playwright-ci-postgresql (2, 6) Failure3 of 359 frontend UI tests failed. Unrelated to backend Elasticsearch/OpenSearch client upgrade. integration-tests-postgres-opensearch FailureAll 9,017 backend integration tests passed (0 failures, 0 errors), but build failed due to database constraint errors in workflow code. Unrelated to search client changes. py-run-tests Failures (3.10 and 3.11)Python integration test failures. This PR modifies only Java files and does not modify any Python code. Completely unrelated to this PR. maven-postgresql-ci & maven-sonarcloud-ci Failures5 backend test errors out of 7,918 tests (99.94% pass rate): 3 AWS Credentials Tests: 2 Workflow Timeout Tests: The AWS credential test failures may be related to AWS credential handling changes merged from main (multiple AWS IAM-related commits in branch history). The workflow timeout errors appear to be test environment timing issues unrelated to search client changes. DetailsMost failures are external to this PR's core functionality. The Elasticsearch/OpenSearch client upgrade demonstrates 99.94% backend test pass rate (7,913 of 7,918 tests passed) with the 5 errors being AWS credential validation tests and workflow timing issues, neither directly related to the search client migration itself. Code Review 👍 Approved with suggestions 8 resolved / 12 findingsMajor client library upgrade (ES 9.x, OS 3.x, HC5 transport) is well-executed. One potential NPE in OpenMetadataOperations where getNewClient() may return null, and OpenSearch generic Response objects should ideally be closed to prevent connection pool exhaustion.
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
Rest5ClientandRest5ClientTransportfor Apache HttpClient 5.x compatibilityApacheHttpClient5Transportreplacing deprecatedRestClienttransportPoolingAsyncClientConnectionManagerBuilderpatternSigV4Hc5RequestSigningInterceptorprovides HC5-compatible AWS IAM authentication for managed Elasticsearch/OpenSearchIllegalStateExceptionwhen target host is unavailable instead of silently skipping signingUNSIGNED-PAYLOADheader (AWS skips body verification over HTTPS)buildHttpHostsForHc5inSearchUtils.javaconstructs HC5HttpHostarrays from configurationElasticSearchClientandOpenMetadataOperationsThis will update automatically on new commits.