-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Refactor registerRepository method #108788
Refactor registerRepository method #108788
Conversation
Pinging @elastic/es-distributed (Team:Distributed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better. I left some suggestions but nothing substantial
.<Void>newForked(validationStep -> { | ||
validateRepositoryCanBeCreated(request); | ||
validationStep.onResponse(null); | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically unnecessary, but using ActionListener.completeWith
makes it more obvious to the casual reader that this listener is always completed:
.<Void>newForked(validationStep -> { | |
validateRepositoryCanBeCreated(request); | |
validationStep.onResponse(null); | |
}) | |
.<Void>newForked(validationStep -> ActionListener.completeWith(validationStep, () -> { | |
validateRepositoryCanBeCreated(request); | |
return null; | |
})) |
publicationStep.addListener(clusterUpdateStep.delegateFailureAndWrap((ignored1, changed) -> { | ||
acknowledgementStep.addListener(clusterUpdateStep.delegateFailureAndWrap((ignored2, ack) -> { | ||
clusterUpdateStep.onResponse(new RegisterRepositoryTaskResult(ack, changed)); | ||
})); | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best not to capture the listener in the lambdas (probably doesn't matter here, we're capturing other things, but still good practice). Also delegateFailure(onResponse(...))
is just map(...)
(the AndWrap
bit shouldn't matter here).
publicationStep.addListener(clusterUpdateStep.delegateFailureAndWrap((ignored1, changed) -> { | |
acknowledgementStep.addListener(clusterUpdateStep.delegateFailureAndWrap((ignored2, ack) -> { | |
clusterUpdateStep.onResponse(new RegisterRepositoryTaskResult(ack, changed)); | |
})); | |
})); | |
publicationStep.addListener( | |
clusterUpdateStep.delegateFailureAndWrap( | |
(clusterUpdateStep1, changed) -> acknowledgementStep.addListener( | |
clusterUpdateStep1.map(ack -> new RegisterRepositoryTaskResult(ack, changed)) | |
) | |
) | |
); |
server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java
Show resolved
Hide resolved
} | ||
}) | ||
// When verification has completed, get the repository data for the first time | ||
.<RepositoryData>andThen((getRepositoryDataStep, ignored) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: unnecessary {...}
block, this can just be an expression lambda.
.<RepositoryData>andThen((getRepositoryDataStep, ignored) -> { | |
.<RepositoryData>andThen((getRepositoryDataStep, ignored) -> |
); | ||
}) | ||
// When the repository metadata is ready, update the repository UUID stored in the cluster state, if available | ||
.<Void>andThen((updateRepoUuidStep, repositoryData) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: unnecessary {...}
block, this can just be an expression lambda.
.<Void>andThen((updateRepoUuidStep, repositoryData) -> { | |
.<Void>andThen((updateRepoUuidStep, repositoryData) -> |
@DaveCTurner thank you for suggestions. Updated PR except verify condition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR is a syntactic change for
registerRepository
inRepositoriesService
.I use
SubscribableListener
to display order of events and reduce boilerplatecode around failures delegation
listener.delegateFailureAndWrap
.It's a part of larger change for verification logic, which should take advantage
of this "sequential" version of code. #108531