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

Delegate TransportUpdateAction to TransportBulkAction #17679

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

msfroh
Copy link
Collaborator

@msfroh msfroh commented Mar 25, 2025

Description

This is consistent with TransportIndexAction and TransportDeleteAction, and ensures that updates behave consistently, whether sent via the _update API or via the _bulk API.

Related Issues

Resolves #16980

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions github-actions bot added >breaking Identifies a breaking change. enhancement Enhancement or improvement to existing feature or request Indexing Indexing, Bulk Indexing and anything related to indexing v3.0.0 Issues and PRs related to version 3.0.0 labels Mar 25, 2025
Copy link
Contributor

❌ Gradle check result for a432e47: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for a432e47: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

private final NodeClient client;
private final ClusterService clusterService;
@Deprecated
public class TransportUpdateAction extends HandledTransportAction<UpdateRequest, UpdateResponse> {
Copy link
Member

Choose a reason for hiding this comment

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

Could we remove this class all together? Seems to not be widely used: https://github.com/search?q=org%3Aopensearch-project%20TransportUpdateAction%20NOT%20is%3Aarchived&type=code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, the @Deprecated annotation was copied from TransportIndexAction and TransportDeleteAction.

In practice, I don't think I actually want to remove it. I should probably remove the annotation. (We should probably remove the annotations from TransportIndexAction and TransportDeleteAction since I don't think they're ever going to be removed either.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There -- I removed this annotation and the ones on TransportIndexAction, TransportDeleteAction, and TransportSingleItemBulkAction.

Copy link
Contributor

❌ Gradle check result for 22a2df3: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for c943a65:

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for c943a65:

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for c943a65: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

msfroh added 5 commits March 28, 2025 11:20
This is consistent with TransportIndexAction and TransportDeleteAction,
and ensures that updates behave consistently, whether sent via the
`_update` API or via the `_bulk` API.

Signed-off-by: Michael Froh <[email protected]>
Signed-off-by: Michael Froh <[email protected]>
Since this change modifies how updates behave with ingest pipelines,
the tests need to be updated to reflect the new behavior (where single
item updates behave the same as bulk updates).

Signed-off-by: Michael Froh <[email protected]>
… and children

In practice, these adapter classes are unlikely to be removed, unless we
drop support (at the API level) for the single-item operations. Since
the single item operations are likely to stick around, we have no plan
to remove these adapter classes.

Signed-off-by: Michael Froh <[email protected]>
Signed-off-by: Michael Froh <[email protected]>
Copy link
Contributor

❌ Gradle check result for ffda134: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking Identifies a breaking change. enhancement Enhancement or improvement to existing feature or request Indexing Indexing, Bulk Indexing and anything related to indexing v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Cleanup] TransportUpdateAction should extend TransportSingleItemBulkWriteAction
3 participants