cassandra icon indicating copy to clipboard operation
cassandra copied to clipboard

CASSANDRA-15254 Support updates over SettingsTable (prorperty validation with DatabaseDescriptor)

Open Mmuzaf opened this issue 1 year ago • 1 comments

Thanks for sending a pull request! Here are some tips if you're new here:

  • Ensure you have added or run the appropriate tests for your PR.
  • Be sure to keep the PR description updated to reflect all changes.
  • Write your PR title to summarize what this PR proposes.
  • If possible, provide a concise example to reproduce the issue for a faster review.
  • Read our contributor guidelines
  • If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

Mmuzaf avatar Apr 28 '23 09:04 Mmuzaf

I wonder why go down this route when we could offer a simpler solution.

yaml currently handles everything but validation/listeners, so could decorate the Properties with a new validation and listener properties

public interface Listener<A, B>
    {
        default void onPreSet(A object, B value) {}
        default void onPostSet(A object, B value) {}
    }

    public static class CompositeListener<A, B> implements Listener<A, B>
    {
        private final List<Listener<A, B>> listeners;

        public CompositeListener(List<Listener<A, B>> listeners)
        {
            this.listeners = listeners;
        }

        @Override
        public void onPreSet(A object, B value)
        {
            for (Listener<A, B> l : listeners)
                l.onPreSet(object, value);
        }

        @Override
        public void onPostSet(A object, B value)
        {
            for (Listener<A, B> l : listeners)
                l.onPostSet(object, value);
        }
    }

    public interface Validator<A, B> extends Listener<A, B>
    {
        void validate(B value);
        default void validate(A object, B value)
        {
            validate(value);
        }

        @Override
        default void onPreSet(A object, B value)
        {
            validate(object, value);
        }
    }

    public static class ListenableProperty<A, B> extends ForwardingProperty
    {
        private final Listener<A, B> listener;
        public ListenableProperty(String name, Property property, Listener<A, B> listener)
        {
            super(name, property);
            this.listener = listener;
        }

        @Override
        public void set(Object o, Object o1) throws Exception
        {
            listener.onPreSet((A) o, (B) o1);
            super.set(o, o1);
            listener.onPostSet((A) o, (B) o1);
        }
    }

We need something to hold the Map<String, Property>, so flexible where this is, but feel like DatabaseDescriptor` is likely the best place.

With this you only worry about Property and don't need to worry about validation/notification as the property was decorated to add such capabilities (and don't pay the cost to check this if nothing validates or listens)... We also can set this up in DatabaseDescriptor so its immutable (we don't need mutability with this), which mostly solves the mapping problem (as DD knows everything, so can build this).

With this, we keep the logic small, 100% align with YAML (can't possibly drift as YAML is what does the string mapping), and solve the "how does the framework" know problem as we push this to DatabaseDescriptor, the place that actually knows about this.

dcapwell avatar May 01 '23 17:05 dcapwell