CASSANDRA-15254 Support updates on the virtuabl table SettingsTable (use annotations for property updates validation) #2334

Mmuzaf · 2023-05-14T15:08:21Z

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

dcapwell · 2023-05-16T17:58:43Z

src/java/org/apache/cassandra/config/Config.java

@@ -204,6 +205,8 @@ public MemtableOptions()
    // Limit the maximum depth of repair session merkle trees
    @Deprecated
    public volatile Integer repair_session_max_tree_depth = null;
+    @Mutable
+    @Validate(useClass = DATABASE_DESCRIPTOR_CLASS, useClassMethod = "validateRepairSessionSpace")


do you do string because Class<DatabaseDescriptor> would cause a circular ref?

src/java/org/apache/cassandra/config/DatabaseDescriptor.java

dcapwell · 2023-05-16T18:36:10Z

src/java/org/apache/cassandra/config/Validate.java

+ * a custom validator class and its method.
+ */
+@Retention(RetentionPolicy.RUNTIME)
+@Target(ElementType.FIELD)


for now this is fine, if we need to can allow in method later, just need to update the loader

src/java/org/apache/cassandra/config/Validate.java

src/java/org/apache/cassandra/config/YamlConfigurationLoader.java

src/java/org/apache/cassandra/config/Validate.java

src/java/org/apache/cassandra/db/virtual/SettingsTable.java

…tation

src/java/org/apache/cassandra/config/ListenableProperty.java

src/java/org/apache/cassandra/config/Config.java

src/java/org/apache/cassandra/config/DataRateSpec.java

src/java/org/apache/cassandra/config/DefaultLoader.java

src/java/org/apache/cassandra/config/ListenableProperty.java

src/java/org/apache/cassandra/config/DatabaseDescriptor.java

…by list

src/java/org/apache/cassandra/config/ConfigFields.java

src/java/org/apache/cassandra/config/DatabaseDescriptor.java

src/java/org/apache/cassandra/config/ValidatedBy.java

src/java/org/apache/cassandra/config/ValidatedPropertyLoader.java

Mmuzaf · 2023-07-03T18:03:58Z

@smiklosovic Thank you for the comments.
Do you agree with the proposed solution design? Do you have any comments on that?

smiklosovic · 2023-07-03T19:08:05Z

@Mmuzaf I think you did a good job and David already saw it too so ... I may go through it next days again but seems robust enough to me! It would be nice to have yet another pair of eyes on this, if you happen to get somebody to take a look, that would be great!

src/java/org/apache/cassandra/config/ListenableProperty.java

smiklosovic · 2023-09-04T20:51:33Z

src/java/org/apache/cassandra/db/virtual/SettingsTable.java

+        try
+        {
+            Object value = getPropertyAsString(getKeyAndWarnIfObsolete(name));
+            result.row(name).column(VALUE, value).column(MUTABLE, DatabaseDescriptor.isMutableProperty(name));


@Mmuzaf I was thinking about this some time ago and I found some time to go over this again ... I think that isMutableProperty is too much here in such sense that it might be "cached". We are not going to introduce a new property while a node is running - it is a static set of keys and values. You can populate a map in a constructor of vtable. So it would be just a simple map lookup, or even easier, names of all mutable properties would be in a set. So if it is not in s set it is not mutable. You do not need to do reflection every single query for every single property all over again.

@smiklosovic agree, this makes sense to me. I'll try to figure out the best way to avoid using reflection every time we need to get the mutability state.

dcapwell · 2023-09-12T18:48:01Z

Sorry, been away from this patch for a very long time. If 2 committers are +1 to this patch please don't wait for me and feel free to merge. I don't know when I will have time to review again.

smiklosovic · 2023-12-19T15:33:43Z

src/java/org/apache/cassandra/db/virtual/SettingsTable.java

                           .build());
-        this.config = config;
+        for (String name : BACKWARDS_COMPATABLE_NAMES.keySet())


@Mmuzaf COMPATIBLE, not COMPATABLE

smiklosovic · 2023-12-19T15:40:29Z

src/java/org/apache/cassandra/io/sstable/indexsummary/IndexSummaryManager.java

+    private void handleResizeIntervalInMinutes(DurationSpec.IntMinutesBound oldValue, DurationSpec.IntMinutesBound newValue)
+    {
+        int oldInterval = getDurationInMinutes(oldValue);
+        int resizeIntervalInMinutes = getDurationInMinutes(newValue);


@Mmuzaf where was this property taken from before? I see it is already used in this method further down bellow ...

smiklosovic · 2023-12-19T15:42:20Z

src/java/org/apache/cassandra/io/sstable/indexsummary/IndexSummaryManager.java

@@ -296,6 +318,7 @@ public void shutdownAndWait(long timeout, TimeUnit unit) throws InterruptedExcep
            future.cancel(false);
            future = null;
        }
+        shutdownTasks.forEach(Runnable::run);


what if Runnable#run throws? Then it will never get to ExecutorUtils.shutdownAndWait on the next line?

smiklosovic · 2023-12-19T15:49:08Z

test/unit/org/apache/cassandra/db/virtual/UpdateSettingsTableTest.java

+    }
+
+    @Test
+    public void testBactchUpdateSettings() throws Throwable


testBatch...

smiklosovic · 2023-12-19T15:53:11Z

src/java/org/apache/cassandra/db/virtual/SettingsTable.java

-    protected static final Map<String, Property> PROPERTIES = ImmutableMap.copyOf(getProperties());
-
-    private final Config config;
+    private static final String MUTABLE = "is_mutable";


@Mmuzaf can be this renamed just to mutable? I think that is just enough ... no need to put there is_.

smiklosovic · 2023-12-19T16:10:04Z

src/java/org/apache/cassandra/config/ValidatedPropertyLoader.java

+                validatedByArray = new ValidatedBy[]{ field.getAnnotation(ValidatedBy.class) };
+        }
+        else
+            validatedByArray = validatedByList.value();


@Mmuzaf could it happen that validatedByList would not be null but the array would be null? Then the next for would loop over null?

Mmuzaf added 14 commits May 10, 2023 20:09

wip

709d7dd

wip

8491bd1

wip

5a2e2d0

Merge branch 'trunk' into cassandra-15254-ann

e100ed6

wip

07d74d3

wip

c231a77

wip

3c891cf

wip

cc39a8d

wip

7863dbf

wip

64fdac0

Merge branch 'trunk' into cassandra-15254-ann

a0c5615

wip

c840fad

wip

e82deab

wip

84f39f3

dcapwell requested changes May 16, 2023

View reviewed changes

Mmuzaf added 6 commits May 17, 2023 16:16

remove unnecessary backwards compatible visitor usage

d4b5539

fix type description registration, fix unnecessary proerty utils set

d39df97

class and method for validation bust be mandatory in the related anno…

2e02f7e

…tation

withReplacementLoader must use getReplacement directly

6c4b1ee

minor fix

21f24c6

use setProperty right from apply methods to set values

b00eafe

dcapwell reviewed May 17, 2023

View reviewed changes

src/java/org/apache/cassandra/config/ListenableProperty.java Outdated Show resolved Hide resolved

Mmuzaf added 6 commits May 20, 2023 17:41

use @validated on pre-parse and on post-parse yaml

3212041

update tests for property validation, code cleanup

61e7527

add after change listener for index_summary property

31d6d75

handle exceptions for jmx

a983ef0

reuse exceptions handling for running configuration udpates

e071dcf

Merge branch 'trunk' into cassandra-15254-ann

08572b5

dcapwell requested changes May 22, 2023

View reviewed changes

added validation listeners for all types of methods, added validated …

9c8a175

…by list

smiklosovic reviewed Jun 26, 2023

View reviewed changes

src/java/org/apache/cassandra/config/ConfigFields.java Outdated Show resolved Hide resolved

smiklosovic reviewed Jun 26, 2023

View reviewed changes

src/java/org/apache/cassandra/config/DatabaseDescriptor.java Outdated Show resolved Hide resolved

smiklosovic reviewed Jun 26, 2023

View reviewed changes

src/java/org/apache/cassandra/config/ValidatedBy.java Show resolved Hide resolved

smiklosovic reviewed Jun 26, 2023

View reviewed changes

src/java/org/apache/cassandra/config/ValidatedPropertyLoader.java Show resolved Hide resolved

Mmuzaf added 2 commits July 3, 2023 17:50

Fix review comments

bb111fc

Merge branch 'trunk' into cassandra-15254-ann

55d1577

smiklosovic reviewed Jul 3, 2023

View reviewed changes

src/java/org/apache/cassandra/config/ListenableProperty.java Outdated Show resolved Hide resolved

Mmuzaf added 8 commits July 4, 2023 16:04

Move ConfigFields to inner class, improve javadoc comments

e69ce61

Merge branch 'trunk' into cassandra-15254-ann

2a75195

a new column added to SettingsTable

e937f89

change column type

f4bbe04

fix checkstyle

d7eb284

Merge branch 'trunk' into cassandra-15254-ann

4ca0b51

Merge branch 'trunk' into cassandra-15254-ann

7a6da67

Merge branch 'trunk' into cassandra-15254-ann

ce213bd

smiklosovic reviewed Sep 4, 2023

View reviewed changes

Mmuzaf added 3 commits September 14, 2023 18:59

Merge branch 'trunk' into cassandra-15254-ann

a39c408

Add caching to the configuration property mutability param

820adab

Merge branch 'trunk' into cassandra-15254-ann

52ab6b7

smiklosovic reviewed Dec 19, 2023

View reviewed changes

belliottsmith force-pushed the trunk branch 2 times, most recently from df3eb40 to 54e39a9 Compare July 23, 2025 11:19

CASSANDRA-15254 Support updates on the virtuabl table SettingsTable (use annotations for property updates validation) #2334

Are you sure you want to change the base?

CASSANDRA-15254 Support updates on the virtuabl table SettingsTable (use annotations for property updates validation) #2334

Uh oh!

Conversation

Mmuzaf commented May 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mmuzaf commented Jul 3, 2023

Uh oh!

smiklosovic commented Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

smiklosovic Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcapwell commented Sep 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smiklosovic Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smiklosovic commented Jul 3, 2023 •

edited

Loading

smiklosovic Sep 4, 2023 •

edited

Loading

smiklosovic Dec 19, 2023 •

edited

Loading