Improve autocluster#19
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors autocluster discovery strategy selection to be hook-driven, updates vote (2PC) callbacks/tracing to include the vote ID, and aligns tests/docs with these API changes.
Changes:
- Introduce
classy_discovery_strategy:hook/2-based discovery method registration and addhook/0to built-in strategies (static/dns/k8s/etcd). - Extend vote callbacks so
prepare/commit/rollback/post_votereceive the vote ID; add participant “recv_outcome” tracing and update properties/docs accordingly. - Simplify discovery test suites by removing now-irrelevant lock/register tests and adjust a membership test to wait for joins.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/classy_SUITE.erl | Updates membership test and vote test callbacks to accept vote ID. |
| test/classy_discovery_static_SUITE.erl | Removes lock/unlock/register/unregister testcases for static strategy. |
| test/classy_discovery_k8s_SUITE.erl | Removes lock/unlock/register/unregister testcases (not applicable anymore). |
| test/classy_discovery_dns_SUITE.erl | Removes lock/unlock/register/unregister testcases for DNS strategy. |
| src/discovery/classy_discovery_static.erl | Adds hook/0 registration; removes optional callbacks. |
| src/discovery/classy_discovery_k8s.erl | Adds hook/0 registration; removes optional callbacks. |
| src/discovery/classy_discovery_etcd.erl | Adds hook/0 registration for etcd discovery. |
| src/discovery/classy_discovery_dns.erl | Adds hook/0 registration; removes optional callbacks. |
| src/classy_vote.erl | Adds detailed moduledoc; adjusts MFA verification for extra vote-id args; updates trace property. |
| src/classy_vote_participant.erl | Prepends vote ID to commit/rollback actions; emits outcome trace with site/id. |
| src/classy_vote_coordinator.erl | Prepends vote ID to post_vote callback invocation. |
| src/classy_internal.hrl | Adds trace kind macro for participant recv_outcome. |
| src/classy_hook.erl | Registers built-in discovery strategies during hook initialization. |
| src/classy_discovery_strategy.erl | Adds hook/2, resolves configured method via hooks, makes lock/register callbacks optional. |
| src/classy_builtin_hooks.erl | Consolidates peer-connection change logging and varies log level based on locality. |
| src/classy_autocluster.erl | Switches to resolved {Module, Options} strategy selection (no manual module name construction). |
| doc/classy.texi | Updates documentation for discovery strategy configuration and hook-based extensibility. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ieQu1
force-pushed
the
dev/improve-autocluster
branch
from
6b50c5d to
091f84c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/classy_vote.erl:203
options()currently typespost_voteandon_failas singlemfargs(), butwith_defaults/1normalizes both to lists ([PostVote]/[OnFail]) and the coordinator expects lists. Consider widening the type to accept both forms to match actual usage.
-type options() ::
#{ tag := tag()
, actions := #{classy:site() => actions()}
, post_vote => classy_lib:mfargs()
, strategy => strategy()
, on_fail => classy_lib:mfargs()
}.
Comment on lines
55
to
+80
| -doc """ | ||
| Read value of @ref{discovery_strategy} environment variable (with default). | ||
| Register a discovery strategy. | ||
|
|
||
| Callbacks registered here match on the @ref{discovery_strategy} configuration | ||
| and return callback module implementing @code{classy_discovery_strategy} behavior. | ||
|
|
||
| The first hook that returns @code{@{ok, Module@}} wins | ||
| and handles all the callbacks for the next discovery cycle. | ||
| """. | ||
| -spec hook(fun(({atom(), options()}) -> {ok, module()} | undefined), classy_hook:prio()) -> classy_hook:hook(). | ||
| hook(Fun, Prio) when is_function(Fun, 1), is_number(Prio) -> | ||
| classy_hook:insert(?hook, Fun, Prio). | ||
|
|
||
| -doc """ | ||
| Read @ref{discovery_strategy} environment variable and decide which strategy to use. | ||
| """. | ||
| -spec get() -> t() | undefined. | ||
| get() -> | ||
| application:get_env(classy, discovery_strategy, {manual, []}). | ||
| Conf = application:get_env(classy, discovery_strategy, {manual, []}), | ||
| case classy_hook:first_match(?hook, [Conf]) of | ||
| {ok, Module} -> | ||
| {_Method, Options} = Conf, | ||
| {Module, Options}; | ||
| undefined -> | ||
| undefined | ||
| end. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.