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

Implement update by query #1644

Merged
merged 1 commit into from
Jan 23, 2021

Conversation

ffaoudi
Copy link
Contributor

@ffaoudi ffaoudi commented Jan 13, 2021

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our issue tracker.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

Closes #1446

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

@sothawo sothawo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1446 is about adding the update by query feature to the library. This means that both ReactiveDocumentOperations and DocumentOperations and their implementations (both on-reactive and reactive) must have either their update methods enhanced to support this case.

Your PR only adds code to the reactive client and not the the operations/templates.

@ffaoudi
Copy link
Contributor Author

ffaoudi commented Jan 13, 2021

Hi @sothawo, for DocumentOperations and ReactiveDocumentOperations, i see that input and output are spring data type not elasticsearch data type, this mean that for the update by query i need to create a new type and transforme it to the UpdateByQueryRequest elasticsearch type?

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

sothawo commented Jan 14, 2021

Yes, we do not want to leak Elasticsearch types into the Spring Data Elasticsearch API. Although there are a few places where this happens, returning Aggregations for example. These places are either old code that has not been fixed or where we did not have the resources yet to change that.

For new additions we should not do this.

About how a solution could be done, check my comment in the issue and let me know what you think about it.

Copy link
Collaborator

@sothawo sothawo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good, thanks a lot for your work. I made some remarks.

Can you please rebase your branch onto master and force push it? I had a PR merged that deals with custom routing and I want to see how this fits together with the routing for the updates

return updateByQueryRequest;
}

public UpdateByQueryRequestBuilder updateByQueryRequestBuilderFor(Client client, UpdateQuery query, IndexCoordinates index) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public UpdateByQueryRequestBuilder updateByQueryRequestBuilderFor(Client client, UpdateQuery query, IndexCoordinates index) {
public UpdateByQueryRequestBuilder updateByQueryRequestBuilder(Client client, UpdateQuery query, IndexCoordinates index) {

please remove these For suffixes. Don't know why I named this function that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return new BulkByScrollResponseBuilder();
}

public static BulkByScrollResponse translateFrom(org.elasticsearch.index.reindex.BulkByScrollResponse bulkByScrollResponse) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static BulkByScrollResponse translateFrom(org.elasticsearch.index.reindex.BulkByScrollResponse bulkByScrollResponse) {
public static BulkByScrollResponse from(org.elasticsearch.index.reindex.BulkByScrollResponse bulkByScrollResponse) {

We name such factory methods normally just from(...) or of(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done for of(...)

Comment on lines 543 to 549
Params params = new Params(request)
.withRouting(updateByQueryRequest.getRouting())
.withPipeline(updateByQueryRequest.getPipeline())
.withRefresh(updateByQueryRequest.isRefresh())
.withTimeout(updateByQueryRequest.getTimeout())
.withWaitForActiveShards(updateByQueryRequest.getWaitForActiveShards())
.withRequestsPerSecond(updateByQueryRequest.getRequestsPerSecond())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to have this formatting, you need to add // to the end of every line. Otherwise the formatter settings will set it back to the longer lines.

I would prefer to change the formatter, but that's the Spring Data style.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

@sothawo sothawo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two potential places with wrong code, otherwise minor stuff

@@ -1404,7 +1398,7 @@ public UpdateRequest updateRequest(UpdateQuery query, IndexCoordinates index) {
if (params == null) {
params = new HashMap<>();
}
Script script = new Script(ScriptType.INLINE, query.getLang(), query.getScript(), params);
Script script = new Script(getScriptType(ScriptType.INLINE), query.getLang(), query.getScript(), params);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't that be

Suggested change
Script script = new Script(getScriptType(ScriptType.INLINE), query.getLang(), query.getScript(), params);
Script script = new Script(getScriptType(query.getScriptType()), query.getLang(), query.getScript(), params);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -1478,7 +1472,7 @@ public UpdateRequestBuilder updateRequestBuilderFor(Client client, UpdateQuery q
if (params == null) {
params = new HashMap<>();
}
Script script = new Script(ScriptType.INLINE, query.getLang(), query.getScript(), params);
Script script = new Script(getScriptType(ScriptType.INLINE), query.getLang(), query.getScript(), params);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Script script = new Script(getScriptType(ScriptType.INLINE), query.getLang(), query.getScript(), params);
Script script = new Script(getScriptType(query.getScriptType(), query.getLang(), query.getScript(), params);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


private UpdateByQueryResponse(long took, boolean timedOut, long total, long updated, long deleted, int batches,
long versionConflicts, long noops, long bulkRetries, long searchRetries,
String reasonCancelled, List<Failure> failures) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
String reasonCancelled, List<Failure> failures) {
@Nullable String reasonCancelled, List<Failure> failures) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* @param failure {@link BulkItemResponse.Failure} to translate
* @return a new {@link Failure}
*/
public static Failure translateFrom(BulkItemResponse.Failure failure) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static Failure translateFrom(BulkItemResponse.Failure failure) {
public static Failure of(BulkItemResponse.Failure failure) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@sothawo sothawo merged commit 0ee0164 into spring-projects:master Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement update by query [DATAES-873]
4 participants