Elasticsearch docs update #42422

lasteris · 2024-08-08T20:48:06Z

Added code snippet about Bulk API usage via low level RestClient.

I've thought, that this is the important thing, which needs to be described in docs, while It's not so obvious how to use (official docs demonstrate only Java API Bulk API usage).
User should know how to do it, till this is really convenient way to index data with the best performance.

In code snippet, you can see ByteArrayEntity. I've tried InputStreamEntity instead, but got Stream already closed error.

marko-bekhta

Hey, thanks for the suggestion!

Since bulk API allows more operations than just adding documents to the index, I'm thinking that it probably would be better to update the example a bit more ....

What if instead of receiving the file/input stream at the Resource level, we accept a lit of fruits, pass them to the service and let the service create a bulk x-ndjson request.

Accepting a file as an input, opens up the possibility to do any kind of operations, which may be too much ...

We also should probably add a corresponding example in Java API as well. Even though, as you've mentioned the Elasticsearch guide already has that example, the Quarkus docs suggest this just above:

if you want to use the Java API client instead, follow the instructions in the Using the Elasticsearch Java Client paragraph instead

so we'd want to keep the two examples perform the same operations...

lasteris · 2024-08-09T11:24:29Z

Hey, thanks for the suggestion!

Since bulk API allows more operations than just adding documents to the index, I'm thinking that it probably would be better to update the example a bit more ....

What if instead of receiving the file/input stream at the Resource level, we accept a lit of fruits, pass them to the service and let the service create a bulk x-ndjson request.

Accepting a file as an input, opens up the possibility to do any kind of operations, which may be too much ...

We also should probably add a corresponding example in Java API as well. Even though, as you've mentioned the Elasticsearch guide already has that example, the Quarkus docs suggest this just above:

if you want to use the Java API client instead, follow the instructions in the Using the Elasticsearch Java Client paragraph instead

so we'd want to keep the two examples perform the same operations...

Ok, why not, i'll do it )

lasteris · 2024-08-10T23:23:12Z

@marko-bekhta made some changes, feel free to review 🤗

lasteris · 2024-08-11T10:47:39Z

Sorry for that. I've done a couple of typos and have not noticed it before commit. Now it fixed, I think...
I've tested code in my project, but copied code inattentively. I hope next time such bad thing not happen to me(

marko-bekhta

Hey, thanks for the updates. You've made good progress on this one.
I didn't notice that the services in the corresponding integration tests were unmodified. I'm sorry about that. I've pointed to those in the inline comments, along with some more suggestions.
Also, please squash your commits once you are done with the updates. Thanks!

marko-bekhta · 2024-08-13T08:55:15Z

docs/src/main/asciidoc/elasticsearch.adoc

@@ -151,13 +174,49 @@ public class FruitService {
        restClient.performRequest(request); //<4>
    }

+    public void indexBulk(List<Fruit> list) throws IOException {


You could find the corresponding source files for these services in

quarkus/integration-tests/elasticsearch-rest-client/src/main/java/io/quarkus/it/elasticsearch/FruitService.java

Line 19 in 7bbbfe2

public class FruitService {

and

quarkus/integration-tests/elasticsearch-java-client/src/main/java/io/quarkus/it/elasticsearch/java/FruitService.java

Line 18 in 7bbbfe2

public class FruitService {

could you add the code there, and then just copy it back to the guide, that way you'll be sure that all works ok.

Yeah, sorry... i've missed anywhere in guideline, that i should touch some tests...
Sure, i'll do it )

marko-bekhta · 2024-08-13T08:58:58Z

docs/src/main/asciidoc/elasticsearch.adoc

+Also create simple utility class, which help us create valid `ndjson`
+for proper work with https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html#docs-bulk-api-desc[Bulk API].
+
+[source, java]
+----
+public class NDJson {
+    private final List<JsonObject> list;
+
+    public NDJson(List<JsonObject> list) {
+        this.list = list;
+    }
+
+    public String encode() {
+        var sb = new StringBuilder();
+        for (JsonObject json : list) {
+            sb.append(json.encode());
+            sb.append("\n");
+        }
+        return sb.toString();
+    }
+}
+----
+


I'd just add a simple util method to the fruit service instead:

private static String toNdJsonString(List<JsonObject> objects) { return objects.stream() .map(JsonObject::encode) .collect(Collectors.joining("\n", "", "\n")); }

the prefix/suffix (2d/3d parameters) are applied to the joined string, so adding the suffix will ensure that there is a new line at the end.

marko-bekhta · 2024-08-13T09:00:36Z

docs/src/main/asciidoc/elasticsearch.adoc

+        for (var fruit : list) {
+
+            entityList.add(new JsonObject().put("index", new JsonObject() //<5>
+                            .put("_index", "fruits").put("_id", observation.id())));


Suggested change

.put("_index", "fruits").put("_id", observation.id())));

.put("_index", "fruits").put("_id", fruit.id)));

marko-bekhta · 2024-08-13T09:05:14Z

docs/src/main/asciidoc/elasticsearch.adoc

+    public void indexBulk(List<Fruit> list) throws IOException {
+
+        var entityList = new ArrayList<JsonObject>();
+
+        for (var fruit : list) {
+
+            entityList.add(new JsonObject().put("index", new JsonObject() //<5>
+                            .put("_index", "fruits").put("_id", observation.id())));
+            entityList.add(JsonObject.mapFrom(fruit));
+        }
+
+        Request request = new Request(
+                "POST", "fruits/_bulk?pretty");
+        request.setEntity(new StringEntity(
+            new NDJson(entityList).encode(), //<6>
+            ContentType.create("application/x-ndjson"))); //pass apropriate Content-Type
+        restClient.performRequest(request);
+    }


Suggested change

public void indexBulk(List<Fruit> list) throws IOException {

var entityList = new ArrayList<JsonObject>();

for (var fruit : list) {

entityList.add(new JsonObject().put("index", new JsonObject() //<5>

.put("_index", "fruits").put("_id", observation.id())));

entityList.add(JsonObject.mapFrom(fruit));

}

Request request = new Request(

"POST", "fruits/_bulk?pretty");

request.setEntity(new StringEntity(

new NDJson(entityList).encode(), //<6>

ContentType.create("application/x-ndjson"))); //pass apropriate Content-Type

restClient.performRequest(request);

}

public void indexBulk(List<Fruit> list) throws IOException {

var entityList = new ArrayList<JsonObject>();

for (var fruit : list) {

entityList.add(new JsonObject().put("index", new JsonObject() //<5>

.put("_index", "fruits").put("_id", observation.id())));

entityList.add(JsonObject.mapFrom(fruit));

}

Request request = new Request("POST", "fruits/_bulk?pretty");

request.setEntity(new StringEntity(

new NDJson(entityList).encode(), //<6>

ContentType.create("application/x-ndjson"))); //pass apropriate Content-Type

restClient.performRequest(request);

}

Let's try keeping the code format similar to what the rest of the service is doing. BTW, once you add the code to the service in the integration tests I've mentioned above and build it, Maven will take care of formatting it for you.

marko-bekhta · 2024-08-13T09:08:29Z

docs/src/main/asciidoc/elasticsearch.adoc

+                "POST", "fruits/_bulk?pretty");
+        request.setEntity(new StringEntity(
+            new NDJson(entityList).encode(), //<6>
+            ContentType.create("application/x-ndjson"))); //pass apropriate Content-Type


Suggested change

ContentType.create("application/x-ndjson"))); //pass apropriate Content-Type

ContentType.create("application/x-ndjson"))); // <7>

....

<7> Pass the content type that is expected by the search backend for bulk requests.

let's be consistent and use the callouts just like you've done in the line above.

marko-bekhta · 2024-08-13T09:12:57Z

docs/src/main/asciidoc/elasticsearch.adoc

+        restClient.performRequest(request);
+    }
+
+    public void delBulk(List<Long> identityList) throws IOException {


Suggested change

public void delBulk(List<Long> identityList) throws IOException {

public void delete(List<Long> identityList) throws IOException {

I'd use simple index(List<Fruit> fruits)/ delete(List<Long> ids) method names. Caller not necessarily need to know that we are using the bulk API to perform the operation.

marko-bekhta · 2024-08-13T09:40:59Z

docs/src/main/asciidoc/elasticsearch.adoc

+<7> Delete operation in Bulk API requires no following body, so we iterate over fruit's identity list and construct body like shown below
+
+[source, json]
+----
+{"delete", {"_index" : "fruits", "_id", "1"}}\n
+{"delete", {"_index" : "fruits", "_id", "2"}}\n
+...           ...             ...        ...\n
+{"delete", {"_index" : "fruits", "_id", "N"}}\n
+----


Suggested change

<7> Delete operation in Bulk API requires no following body, so we iterate over fruit's identity list and construct body like shown below

+

[source, json]

----

{"delete", {"_index" : "fruits", "_id", "1"}}\n

{"delete", {"_index" : "fruits", "_id", "2"}}\n

... ... ... ...\n

{"delete", {"_index" : "fruits", "_id", "N"}}\n

----

<7> The bulk API's delete operation JSON already contains all the required information; hence, there is no request body following this operation in the Bulk API request body.

+

[source, json]

----

{"delete", {"_index" : "fruits", "_id", "1"}}

{"delete", {"_index" : "fruits", "_id", "2"}}

... ... ... ...

{"delete", {"_index" : "fruits", "_id", "N"}}

----

I'd remove the \n from the example and add the new line at the end, as we are not "escaping" any other characters.

marko-bekhta · 2024-08-13T09:46:44Z

docs/src/main/asciidoc/elasticsearch.adoc

+
+        for (var fruit : list) {
+            br.operations(op -> op
+            .index(idx -> idx.index("fruits").id(fruit.id()).document(fruit)));


Suggested change

.index(idx -> idx.index("fruits").id(fruit.id()).document(fruit)));

.index(idx -> idx.index("fruits").id(fruit.id).document(fruit)));

The bean in the example only has public fields.

marko-bekhta · 2024-08-13T09:55:35Z

docs/src/main/asciidoc/elasticsearch.adoc

+        if (result.errors()) {
+            for (BulkResponseItem item: result.items()) {
+                if (item.error() != null) {
+                    Log.error(item.error().reason());
+                }
+            }
+        }


Logging an exception wouldn't let the caller know that something went wrong. For the sake of this example, I'd just throw an exception that something went wrong:

Suggested change

if (result.errors()) {

for (BulkResponseItem item: result.items()) {

if (item.error() != null) {

Log.error(item.error().reason());

}

}

}

if (result.errors()) {

throw new RuntimException("The indexing operation encountered errors.");

}

lasteris · 2024-08-17T21:14:30Z

@marko-bekhta
The changes you requested have been made.
I merged the commits.
And I also have a question.
What is the usual review communication procedure?
Should I reply to every comment?
Or will you look at my changes yourself and clarify the status if you have any questions?

github-actions · 2024-08-17T21:41:44Z

🙈 The PR is closed and the preview is expired.

marko-bekhta · 2024-08-18T16:17:33Z

Thanks for making the changes, I'll try to take a look later today or tomorrow.

Should I reply to every comment?

No, that's not necessary, unless you want to do so. Or if you want to discuss something in more details 🙂

marko-bekhta

Thanks! I think we are nearly there 😃
We'd have to update the ndjson examples as they are missing the new-line at the end; but, otherwise, this looks nice.

marko-bekhta · 2024-08-20T08:24:48Z

docs/src/main/asciidoc/elasticsearch.adoc

+{"id": "N", "name": "dragonfruit", "color": "pink"}
+----


Suggested change

{"id": "N", "name": "dragonfruit", "color": "pink"}

----

{"id": "N", "name": "dragonfruit", "color": "pink"}

----

I think we have to have that "extra" new line at the end as that's what Elasticsearch expects to mark the end of the request body. Without it I think the request is rejected, no? (and the same with the other ndjson example)

marko-bekhta · 2024-08-20T08:30:33Z

...tests/elasticsearch-rest-client/src/main/java/io/quarkus/it/elasticsearch/FruitResource.java

-import jakarta.ws.rs.Path;
-import jakarta.ws.rs.PathParam;
-import jakarta.ws.rs.QueryParam;
+import jakarta.ws.rs.*;


we try to stay away from the star imports 🙈

Sorry, it's mistake. Usually we too..

marko-bekhta · 2024-08-20T08:37:31Z

...s/elasticsearch-rest-client/src/test/java/io/quarkus/it/elasticsearch/FruitResourceTest.java

+        given()
+                .contentType("application/json")
+                .body(List.of(fruit))
+                .when().post("/fruits/bulk")
+                .then()
+                .statusCode(200);
+
+        given()
+                .contentType("application/json")
+                .body(List.of(fruit.id))
+                .when().delete("/fruits/bulk")
+                .then()
+                .statusCode(200);


I probably wouldn't suggest this one, but since we'd have to update the ndjson examples in the doc file ...
Maybe we can create a new fruit index / check it exists / delete / check it doesn't exist anymore ?

see how just above Fruit result = get("/fruits/1").as(Fruit.class); is used to get the fruit (we can use that to check if it exists)
you'll need to wait for the index to refresh so you may want to use awaitlity for that instatead of the Thread.sleep(), e.g.:

await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> { get("/fruits/1").statusCode(200); });

wow, that's nicee

lasteris · 2024-08-24T22:25:20Z

@marko-bekhta I've built locally (My machine on Windows 11)

As can see, tests are successful. In checks, also tests on Windows are ok. What does it mean

Quarkus CI / JVM Tests - JDK 17 (Failed)
Quarkus CI / JVM Tests - JDK 21 (Failed)
Quarkus CI / Native Tests - Data6 (Failed)

And how i can fix it ((

lasteris · 2024-08-28T08:26:12Z

Finally, build successful ) @marko-bekhta
Thank you for helping me throughout all the problems.

lasteris · 2024-08-28T12:00:10Z

Also, i think, got an idea why my code not works but your does.
Am i right that its because whole process need to be wrapped in await, or else we just call search in a middle of indexing process ?

marko-bekhta · 2024-08-29T07:10:24Z

Also, i think, got an idea why my code not works but your does. Am i right that its because whole process need to be wrapped in await, or else we just call search in a middle of indexing process ?

See https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-get.html#realtime and elastic/elasticsearch#48843

Calling get can start returning the document before the index refresh is completed. If the index has not yet been refreshed, the search will not find the document. So it was either put all the asserts in the await block or put the search query in the await and then get by id outside after the search completes successfully.
Having all the asserts together seemed like a nicer option 😃.

It worked okay in the past since there was a thread.sleep() which waited for the refresh to complete before sending requests.

marko-bekhta

Thanks! This looks nice, I've added a couple comments inline, I assume the compiler plugin change was introduced while testing and wasn't removed before the push?

marko-bekhta · 2024-08-29T07:18:46Z

integration-tests/elasticsearch-rest-client/pom.xml

+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-compiler-plugin</artifactId>
+        <configuration>
+          <source>11</source>
+          <target>11</target>
+        </configuration>
+      </plugin>


That's a bit unexpected ...
I'd expect that the compiler plugin configuration would come from the parent poms. We shouldn't override the versions in a test module... was there a particular reason for adding it?

Suggested change

<plugin>

<groupId>org.apache.maven.plugins</groupId>

<artifactId>maven-compiler-plugin</artifactId>

<configuration>

<source>11</source>

<target>11</target>

</configuration>

</plugin>

I did not do this intentionally.
I think I just used some suggestion of IDE and missed, when pushed commit

Inattention. I accepted the IDE's offer, but then overlooked it when reviewing the commit.
I've deleted these lines, and now I am trying to Quarkus build again.

marko-bekhta · 2024-08-29T07:21:00Z

...s/elasticsearch-rest-client/src/test/java/io/quarkus/it/elasticsearch/FruitResourceTest.java


 @QuarkusTest
 public class FruitResourceTest {
    private static final TypeRef<List<Fruit>> LIST_OF_FRUIT_TYPE_REF = new TypeRef<>() {
    };

    @Test
-    public void testEndpoint() throws InterruptedException {
+    public void testEndpoint() {
+        RestAssured.defaultParser = Parser.JSON;


I wonder if we are just not missing a content type header in some request/response ?

I didn't quite understand the meaning of the question. Do you mean that in this context this line makes no sense?

😃 what I was trying to say ... I'd expect that RestAssured figures out the parser on its own, and we don't need to specify one explicitly. But if it fails somewhere with an exception that it cannot parse the body, then maybe we are just missing a @Produces annotation somewhere in our rest endpoint...

Well, year, got your point finally. Thank you for clarification :)

😃 what I was trying to say ... I'd expect that RestAssured figures out the parser on its own, and we don't need to specify one explicitly. But if it fails somewhere with an exception that it cannot parse the body, then maybe we are just missing a @Produces annotation somewhere in our rest endpoint...

I've made all the changes.🌚

Added code snippet about Bulk API usage via low level RestClient expanded Bulk API usage description rewritten low level Rest Client example added Java API example added awaitility added test cases for bulk operations

quarkus-bot · 2024-08-29T11:35:41Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit c679d63.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

You can consult the Develocity build scans.

quarkus-bot · 2024-08-29T11:43:13Z

Status for workflow `Quarkus Documentation CI`

This is the status report for running Quarkus Documentation CI on commit c679d63.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

marko-bekhta

thanks @lasteris 😃
@gsmet I think this one is ready now.

gsmet · 2024-09-30T12:26:44Z

Thanks to both of you!

quarkus-bot bot added the area/documentation label Aug 8, 2024

gsmet requested a review from marko-bekhta August 8, 2024 20:52