Skip to content

Conversation

DaveCTurner
Copy link
Contributor

In #93825 we introduced a CAS operation on snapshot repositories. In due course we'll want to verify that this operation works as expected using the repository analysis API. This commit reworks the implementation slightly so as to simplify the change which will add this functionality.

In elastic#93825 we introduced a CAS operation on snapshot repositories. In due
course we'll want to verify that this operation works as expected using
the repository analysis API. This commit reworks the implementation
slightly so as to simplify the change which will add this functionality.
@DaveCTurner DaveCTurner added :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >refactoring v8.8.0 labels Feb 20, 2023
@DaveCTurner DaveCTurner requested a review from fcofdez February 20, 2023 09:58
@elasticsearchmachine elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Feb 20, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

import java.util.concurrent.Semaphore;
import java.util.function.BiConsumer;

public class ThrottledIterator<T> implements Releasable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class (and its test suite) extracted from #93735.

}
}

private final RefCounted throttleRefs;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I find throttleRefs a bit confusing in this context, for what I understand this reference count does not control the throttling, it's used to notify the listener once all the tasks have finished, maybe we can find a clearer name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, on reflection refs seems better to me.

Copy link
Contributor

@fcofdez fcofdez left a comment

Choose a reason for hiding this comment

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

LGTM, I left a minor comment about a field name. 👍

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 20, 2023
@elasticsearchmachine elasticsearchmachine merged commit fcf8b6d into elastic:main Feb 20, 2023
@DaveCTurner DaveCTurner deleted the 2023-02-20-repo-analysis-generalize branch February 20, 2023 16:52
carlosdelest pushed a commit to carlosdelest/elasticsearch that referenced this pull request Feb 21, 2023
In elastic#93825 we introduced a CAS operation on snapshot repositories. In due
course we'll want to verify that this operation works as expected using
the repository analysis API. This commit reworks the implementation
slightly so as to simplify the change which will add this functionality.
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!) :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >refactoring Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants