Skip to content

Conversation

@DaveCTurner
Copy link
Contributor

Introduces some never-called localOnly() placeholders to clearly mark
the dead code in actions that never run on remote nodes.

Relates #100111

Introduces some never-called `localOnly()` placeholders to clearly mark
the dead code in actions that never run on remote nodes.

Relates elastic#100111
@DaveCTurner DaveCTurner added >non-issue :Core/Infra/Core Core issues without another label v8.11.0 labels Sep 30, 2023
@DaveCTurner DaveCTurner requested a review from a team as a code owner September 30, 2023 09:59
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Sep 30, 2023
@elasticsearchmachine
Copy link
Collaborator

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

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.

LGTM, but I have some questions.

I suspect this covers only a couple of example usages, and we would need to follow up with some other PRs to convert other existing usages, correct?
These replacements look safe as behaviour is unchanged, and probably other direct usages of TransportAction will too (as their implementation is already empty/throwing). But what about HandledTransportAction? Is there a way we can see which ones are local only, and introduce this pattern/a similar pattern?

@DaveCTurner
Copy link
Contributor Author

Is there a way we can see which ones are local only, and introduce this pattern/a similar pattern?

Not easily, the bulk of the work for #100111 is going to be looking at all the other actions one-by-one and separating them out into local-only and remote-cluster actions.

@DaveCTurner DaveCTurner merged commit efbb18e into elastic:main Oct 2, 2023
@DaveCTurner DaveCTurner deleted the 2023/09/30/local-only-actions branch October 2, 2023 07:59
piergm pushed a commit to piergm/elasticsearch that referenced this pull request Oct 2, 2023
Introduces some never-called `localOnly()` placeholders to clearly mark
the dead code in actions that never run on remote nodes.

Relates elastic#100111
@ldematte
Copy link
Contributor

ldematte commented Oct 2, 2023

the bulk of the work for #100111 is going to be looking at all the other actions one-by-one and separating them out into local-only and remote-cluster actions.

I had a suspect that was the case :/
Do you think that "manual inspection" would be enough (e.g. can we isolate which actions are not used with RemoteClusterAwareClient and infer they are local-only) or should we think about a way of validating this first, e.g. by "instrumenting" all actions with logs, run tests/deploy in Staging and collect info?

@DaveCTurner
Copy link
Contributor Author

I think I'd approach it by separating the remote-cluster actions from the local-cluster ones first, using the type system to catch any mistakes. However I would expect this all to be fairly well-covered by tests (particularly REST tests) so as long as the tests pass we're probably ok.

jakelandis pushed a commit to jakelandis/elasticsearch that referenced this pull request Oct 2, 2023
Introduces some never-called `localOnly()` placeholders to clearly mark
the dead code in actions that never run on remote nodes.

Relates elastic#100111
@DaveCTurner DaveCTurner restored the 2023/09/30/local-only-actions branch June 17, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.11.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants