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

[Transform] Make use of delegateFailureAndWrap #106034

Merged
merged 3 commits into from
Mar 7, 2024

Conversation

prwhelan
Copy link
Member

@prwhelan prwhelan commented Mar 6, 2024

Refactoring to a later pattern of ActionListener, reducing memory footprint and removing some redudant lines of code.

Refactoring to a later pattern of ActionListener, reducing memory
footprint and removing some redudant lines of code.
@prwhelan prwhelan added >non-issue :ml/Transform Transform Team:ML Meta label for the ML team v8.14.0 labels Mar 6, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

unusedValidationResponse -> putTransform(request, listener),
listener::onFailure
ActionListener<ValidateTransformAction.Response> validateTransformListener = listener.delegateFailureAndWrap(
(paramListener, unused) -> putTransform(request, paramListener)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also settle on the naming for the lambda parameter, here paramListener. If you look at the recent refactoring PR (#105907), we chose l as the name, although I'm fine with any other name, just please keep it consistent throughout the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Went with l since that is what the previous PR did (I thought it was delegate but was mistaken)

transformConfigManager.putTransformConfiguration(config, putTransformConfigurationListener);
var config = request.getConfig();
transformConfigManager.putTransformConfiguration(config, listener.delegateFailureAndWrap((paramListener, unused) -> {
var configId = config.getId();
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually use transformId for this. Could you use this name here too?

Copy link
Contributor

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

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

LGTM

@prwhelan
Copy link
Member Author

prwhelan commented Mar 6, 2024

@elasticmachine update branch

@prwhelan prwhelan merged commit 422952d into elastic:main Mar 7, 2024
14 checks passed
fang-xing-esql pushed a commit to fang-xing-esql/Elasticsearch that referenced this pull request Mar 8, 2024
* [Transform] Make use of delegateFailureAndWrap

Refactoring to a later pattern of ActionListener, reducing memory
footprint and removing some redudant lines of code.

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml/Transform Transform >non-issue Team:ML Meta label for the ML team v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants