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

ClusterTopologyRefreshOptions.Builder. enableAdaptiveRefreshTrigger (…) without options should throw IllegalArgumentException #2575

Closed
zzmmff opened this issue Dec 25, 2023 · 1 comment
Labels
type: feature A new feature
Milestone

Comments

@zzmmff
Copy link

zzmmff commented Dec 25, 2023

Bug Report

I set the cluster topology refresh options in my code

    ClusterTopologyRefreshOptions clusterTopologyRefreshOptions = ClusterTopologyRefreshOptions.builder()
              .enableAdaptiveRefreshTrigger()
              // 开启定时刷新
              .enablePeriodicRefresh(Duration.ofSeconds(5))
              .build();

The code is wrong,because no param passed to the enableAdaptiveRefreshTrigger method.

The ClusterTopologyRefreshOptions::Builder::enableAdaptiveRefreshTrigger method souce code :

   /**
         * Enables adaptive topology refreshing using one or more {@link RefreshTrigger triggers}. Adaptive refresh triggers
         * initiate topology view updates based on events happened during Redis Cluster operations. Adaptive triggers lead to an
         * immediate topology refresh. Adaptive triggered refreshes are rate-limited using a timeout since events can happen on
         * a large scale. Adaptive refresh triggers are disabled by default. See also
         * {@link #adaptiveRefreshTriggersTimeout(long, TimeUnit)} and {@link RefreshTrigger}.
         *
         * @param refreshTrigger one or more {@link RefreshTrigger} to enabled
         * @return {@code this}
         */
        public Builder enableAdaptiveRefreshTrigger(RefreshTrigger... refreshTrigger) {

            LettuceAssert.notNull(refreshTrigger, "RefreshTriggers must not be null");
            LettuceAssert.noNullElements(refreshTrigger, "RefreshTriggers must not contain null elements");

            adaptiveRefreshTriggers.addAll(Arrays.asList(refreshTrigger));
            return this;
        }

I think the method should throw IllegalArgumentException if I don't pass any param to the method. But enableAdaptiveRefreshTrigger method didn't work like this,if you don't pass any params to the method ,it wouldn't throw any exceptions (Like the case I mentioned).I think this is a bug

Current Behavior

throw IllegalArgumentException

@zzmmff zzmmff changed the title The Null Assert Same Not Wrok In ClusterTopologyRefreshOptions::Builder The Null Assert Do Not Wrok In ClusterTopologyRefreshOptions::Builder Dec 25, 2023
@mp911de mp911de added the type: feature A new feature label Dec 27, 2023
@mp911de mp911de added this to the 6.3.1.RELEASE milestone Dec 27, 2023
@mp911de
Copy link
Collaborator

mp911de commented Dec 27, 2023

Varargs methods are easy to misuse, especially in this context, because it isn't apparent that you should specify values. We can add an exception here to indicate that no argument has been supplied. We have tons of other places that also accept varargs but these lead on command execution to failures.

@mp911de mp911de changed the title The Null Assert Do Not Wrok In ClusterTopologyRefreshOptions::Builder ClusterTopologyRefreshOptions.Builder. enableAdaptiveRefreshTrigger (…) without options should throw IllegalArgumentException Dec 27, 2023
@mp911de mp911de closed this as completed Dec 27, 2023
mp911de added a commit that referenced this issue Dec 27, 2023
…iveRefreshTrigger` is called without options #2575

Improve usage by throwing an exception to early fail on improper API usage.
mp911de added a commit that referenced this issue Dec 27, 2023
…iveRefreshTrigger` is called without options #2575

Improve usage by throwing an exception to early fail on improper API usage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A new feature
Projects
None yet
Development

No branches or pull requests

2 participants