thomasmueller commented on a change in pull request #490:
URL: https://github.com/apache/jackrabbit-oak/pull/490#discussion_r804435058
##########
File path:
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticConnection.java
##########
@@ -90,15 +95,23 @@ private ElasticConnection(@NotNull String indexPrefix,
@NotNull String scheme, @
this.apiKeySecret = apiKeySecret;
}
+ /**
+ * Gets the REST High Level Client and instanciates the NEW Elasticsearch
client
+ * by sharing the REST High Level Client transport layer
+ * to follow the proposed migration strategy:
+ *
https://www.elastic.co/guide/en/elasticsearch/client/java-api-client/7.16/migrate-hlrc.html
+ * It double-checks locking to get good performance and avoid double
initialization
+ * @deprecated
+ * @return the old Elasticsearch client
+ */
public RestHighLevelClient getClient() {
if (isClosed.get()) {
throw new IllegalStateException("Already closed");
}
- // double-checked locking to get good performance and avoid double
initialization
- if (client == null) {
+ if (hlClient == null) {
Review comment:
If we do use double-checked locking, then we need to use esClient ==
null here, otherwise there is a possibility that hlClient != null but esClient
isn't assigned yet (race condition).
And, at some point in the future, I assume we want to get rid of the
hlClient field. So, it's better to not use it where possible.
##########
File path:
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticConnection.java
##########
@@ -90,15 +95,23 @@ private ElasticConnection(@NotNull String indexPrefix,
@NotNull String scheme, @
this.apiKeySecret = apiKeySecret;
}
+ /**
+ * Gets the REST High Level Client and instanciates the NEW Elasticsearch
client
+ * by sharing the REST High Level Client transport layer
+ * to follow the proposed migration strategy:
+ *
https://www.elastic.co/guide/en/elasticsearch/client/java-api-client/7.16/migrate-hlrc.html
+ * It double-checks locking to get good performance and avoid double
initialization
+ * @deprecated
+ * @return the old Elasticsearch client
+ */
public RestHighLevelClient getClient() {
if (isClosed.get()) {
throw new IllegalStateException("Already closed");
}
- // double-checked locking to get good performance and avoid double
initialization
Review comment:
This comment still applies, so I would keep it.
##########
File path:
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexCleaner.java
##########
@@ -138,13 +138,11 @@ public void run() {
danglingRemoteIndicesMap.remove(remoteIndexName);
}
}
- DeleteIndexRequest deleteIndexRequest = new
DeleteIndexRequest(indicesToDelete.toArray(new String[]{}));
- if (deleteIndexRequest.indices() != null &&
deleteIndexRequest.indices().length > 0) {
- String indexString =
Arrays.toString(deleteIndexRequest.indices());
- AcknowledgedResponse acknowledgedResponse =
elasticConnection.getClient().indices().delete(deleteIndexRequest,
RequestOptions.DEFAULT);
- LOG.info("Deleting remote indices {}", indexString);
- if (!acknowledgedResponse.isAcknowledged()) {
- LOG.error("Could not delete remote indices " +
indexString);
+ if(!indicesToDelete.isEmpty()) {
+ DeleteIndexResponse response =
elasticConnection.getElasticsearchClient().indices().delete(i->i.index(indicesToDelete));
Review comment:
Please add spaces around ->
##########
File path:
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticConnection.java
##########
@@ -135,8 +162,8 @@ public boolean isAvailable() {
@Override
public synchronized void close() throws IOException {
- if (client != null) {
- client.close();
+ if (hlClient != null) {
Review comment:
It is slightly weird to check hlClient != null but then use esClient...
I think this would be easier to read:
if (esClient != null) {
esClient._transport().close();
}
##########
File path:
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticConnection.java
##########
@@ -108,11 +121,25 @@ public RestHighLevelClient getClient() {
Header[] headers = new Header[]{new
BasicHeader("Authorization", "ApiKey " + apiKeyAuth)};
builder.setDefaultHeaders(headers);
}
- client = new RestHighLevelClient(builder);
+ hlClient = new RestHighLevelClient(builder);
Review comment:
Let's say there is an exception or error (RuntimeException,
OutOfMemoryError,...) after setting the hlClient but before setting the
esClient. In this case, we end up having hlClient != null and esClient == null,
which is a weird state...
To protect against that, the only 100% save way I would know is encapsulate
esClient + hlClient in a separate object (a "ClientPair" class) and assign
that... but it would be overly complicated I think. An alternative is to do the
following (untested):
RestHighLevelClient rc = new RestHighLevelClient(builder);
ElasticsearchClient ec = new ElasticsearchClient(new RestClientTransport(
rc.getLowLevelClient(), new JacksonJsonpMapper()));
// "atomically" assign to the fields
hlClient = rc;
esClient = ec;
##########
File path:
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticConnection.java
##########
@@ -108,11 +121,25 @@ public RestHighLevelClient getClient() {
Header[] headers = new Header[]{new
BasicHeader("Authorization", "ApiKey " + apiKeyAuth)};
builder.setDefaultHeaders(headers);
}
- client = new RestHighLevelClient(builder);
+ hlClient = new RestHighLevelClient(builder);
+ ElasticsearchTransport transport = new RestClientTransport(
+ hlClient.getLowLevelClient(),
+ new JacksonJsonpMapper()
+ );
+ esClient = new ElasticsearchClient(transport);
}
}
}
- return client;
+ return hlClient;
+ }
+
+ /**
+ * Gets the NEW Elasticsearch Client
+ * @return the new Elasticsearch client
+ */
+ public ElasticsearchClient getElasticsearchClient() {
+ getClient();
+ return esClient;
Review comment:
It looks like a tab character here... In our code (well, my code at
least), we use spaces instead of tabs (4 spaces). Could you configure your IDE
to use spaces instead of tabs? I know it's not the Eclipse default...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
