Skip to content

Conversation

DaveCTurner
Copy link
Contributor

The semantics of ActionType are somewhat confusing; this commit
expands its Javadocs a bit to try and clarify how to use it.

The semantics of `ActionType` are somewhat confusing; this commit
expands its Javadocs a bit to try and clarify how to use it.
@DaveCTurner DaveCTurner requested a review from joegallo January 9, 2024 15:18
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jan 9, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

*
* @param name The name of the action, which must be unique across actions. When executed on a remote cluster, this is the
* ID of the transport action which is sent to the handling node in the remote cluster.
* @param responseReader A reader for the response type, which is only used by actions executed on a remote cluster.
Copy link
Contributor

Choose a reason for hiding this comment

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

A reader for the response type, which is only used by actions executed on a remote cluster.

Is that a reader for the response from the action executed on the remote cluster? I'm trying to read the as-written words incorrectly and I feel like I can convince myself that those words say that it's the action executed on the remote cluster that uses the reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair, I see the ambiguity. Let me rearrange the words, sec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1b8526b any better?

Copy link
Contributor

Choose a reason for hiding this comment

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

SO MUCH BETTER. Thank you! I love it!

@DaveCTurner DaveCTurner requested a review from joegallo January 9, 2024 16:53
Copy link
Contributor

@joegallo joegallo left a comment

Choose a reason for hiding this comment

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

❤️

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jan 10, 2024
Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

Late to the party, but I really like it! It would have saved me some time while I was trying to understand action registration/execution.

@elasticsearchmachine elasticsearchmachine merged commit 25e9f84 into elastic:main Jan 10, 2024
@DaveCTurner DaveCTurner deleted the 2024/01/09/ActionType-comment branch January 10, 2024 09:32
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Jan 26, 2024
Aligns the actions in the enterprise search module with the docs added
in elastic#104139.
DaveCTurner added a commit that referenced this pull request Jan 29, 2024
Aligns the actions in the enterprise search module with the docs added
in #104139.
@DaveCTurner DaveCTurner restored the 2024/01/09/ActionType-comment branch June 17, 2024 06:17
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Oct 20, 2025
No need for these things to be subclasses of `ActionType`, let's save
some metaspace.

Relates elastic#104139
DaveCTurner added a commit that referenced this pull request Oct 20, 2025
No need for these things to be subclasses of `ActionType`, let's save
some metaspace.

Relates #104139
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Transport API Transport client API >non-issue Team:Core/Infra Meta label for core/infra team v8.13.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants