Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential throwing of the blocking exception #2025

Closed
AntonLGVS opened this issue Dec 13, 2021 · 11 comments · Fixed by #2094
Closed

Potential throwing of the blocking exception #2025

AntonLGVS opened this issue Dec 13, 2021 · 11 comments · Fixed by #2094
Labels
status: feedback-provided Feedback has been provided

Comments

@AntonLGVS
Copy link
Contributor

In the DefaultReactiveElasticsearch class there is a call to block(). This could potentially cause the IllegalStateException("block()/blockFirst()/blockLast()...) to be thrown. This will be thrown into EventLoop of the Netty Server.
Can make a reactive implementation of the SearchDocumentResponse to remove the blocking call?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 13, 2021
@sothawo
Copy link
Collaborator

sothawo commented Dec 14, 2021

where exactly in which class?

@sothawo sothawo added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 14, 2021
@AntonLGVS
Copy link
Contributor Author

class ReactiveElasticsearchTemplate:

line 805: Function<SearchDocument, Object> entityCreator = searchDocument -> documentCallback.toEntity(searchDocument)
line 806:     .block();

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 14, 2021
@sothawo
Copy link
Collaborator

sothawo commented Dec 15, 2021

I'll have to have a deeper look at this callback, but I think that the reactive version of this callback is not needed here anyway and this could be a non-reactive call.

@sothawo sothawo added in: mapping Mapping and conversion infrastructure status: under investigation labels Dec 15, 2021
@AntonLGVS
Copy link
Contributor Author

AntonLGVS commented Dec 15, 2021

Maybe change the callback to a non-reactive version? I can do it.

@sothawo
Copy link
Collaborator

sothawo commented Dec 15, 2021

The non-reactive code has some callback for this as well, this should be used or consolidated into common code.

@AntonLGVS
Copy link
Contributor Author

The callback ReadSearchDocumentCallback has a call to the maybeCallAfterConvert public API.
There are solution options:

  1. Implementation of the reactive SearchDocumentResponse;
  2. Calling the maybeCallAfterConvert method outside the ReadSearchDocumentCallback#toEntity method, but then we need to change the signature of the ReadSearchDocumentCallback#toEntity and return a wrapper with parameters for the maybeCallAfterConvert method. A rough example:
Wrapper wr = toEntity(...);
maybeCallAfterConvert(wr.p1(), wr.p2(),.....);

@sothawo
Copy link
Collaborator

sothawo commented Dec 20, 2021

I'll hopefully have a look at this deeper in the next days currently I'm quite busy (not doing this as my day job 😃 )

@AntonLGVS
Copy link
Contributor Author

I'am know that is a free time job. Me too. I have the opportunity to help you develop this project.

@sothawo
Copy link
Collaborator

sothawo commented Dec 23, 2021

seems the best would be like you suggested to have the SearchDocumentResponse reactive. But this class is currently tied to Elasticsearch classes, so we need to find an abstraction that will work for both the old and new client.

@AntonLGVS
Copy link
Contributor Author

If you don't mind, i'll take this problem

@sothawo
Copy link
Collaborator

sothawo commented Dec 23, 2021

that has to be done with the introduction of the new client. I have a branch in my fork where I am working on this; it is planned for the version 4.4 to be able to use either the RestHighLevelClient or the ElasticsearchClient, depending on the configuration. In 5.0 we will then only support the new client.

I can really work on this since Elasticsearch 7.16 is out, since then the new client is fairly stable, before that, there were many changes in packages, structure and functionality.

This also means that we are writing a new reactive template and reactive client as well because we need to use the same request and response classes that the Elasticsearch client uses. For the existing code we had copies/adaptions of code from the Elasticsearch codebase, we need to get rid of that.

So currently I am building a code structure where we for both the imperative and the reactive code we have implementations using the old Elasticsearch libraries and copied code or the new one. In addition to that, that will enable Spring Data Elasticsearch to eventually have other client implementations in the future (OpenSearch might be one).

So this issue with the document callback needs to be addressed in this whole process of restructuring the code. Changing this now in the current code will need refactoring later. So I am not sure where you could currently help here.

sothawo added a commit to sothawo/spring-data-elasticsearch that referenced this issue Feb 20, 2022
sothawo added a commit that referenced this issue Feb 20, 2022
@sothawo sothawo removed status: under investigation in: mapping Mapping and conversion infrastructure labels Feb 20, 2022
@sothawo sothawo added this to the 4.4 M4 (2021.2.0) milestone Feb 20, 2022
sothawo added a commit that referenced this issue Feb 20, 2022
Original Pull Request #2094
Closes #2025

(cherry picked from commit c1a1ea9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants