CNDB-13303: Made warn log regarding obsolete property less spammy. by MSpryszynski · Pull Request #2229 · datastax/cassandra

MSpryszynski · 2026-02-12T17:41:12Z

Made warn log regarding obsolete property less spammy by configuring threshold for repetition per property.

What is the issue

There are reoccurring logs "Ignoring obsolete property nodesync" which provide no context other than that this property is still being used.

Fixes https://github.com/riptano/cndb/issues/13303

What does this PR fix and why was it fixed

Implements mechanism that allow logging single obsolete property once per 30s. It should significantly decrease number of logs and still be equally useful as we do have information that somebody is using this obsolete property.

github-actions · 2026-02-12T17:41:29Z

scottfines

A couple of things:

There is a race condition in the actual implementation, which I detailed in a comment. It shouldn't be too hard to fix.
The testing logic relies on OBSOLETE_PROPERTY_LOG_INTERVAL_MS being non-final, and then uses clock timing to verify the logic. From a production standpoint, this isn't a big deal because we aren't going to change the variable anyway (it's effectively final). But from a testing logic it makes me nervous that we may end up adding an unreliable test, depending on clock timing in CI operating systems. It would be preferable to use some type of abstraction which can be swapped out for testing. On option might be to use a Clock abstraction, which can allow for logical and controlled clock changes. Another might be to push the actual properties logging into a class which can be configured while testing to not require explicit clock timing.

scottfines · 2026-03-20T16:35:30Z

    private static final Pattern NEGATIVE_PATTERN = Pattern.compile("(0|false|no)");
-
+    private static final Map<String, Long> OBSOLETE_PROPERTY_LOG_TIME = new ConcurrentHashMap<>();
+    private static long OBSOLETE_PROPERTY_LOG_INTERVAL_MS = 30_000;


This can be final, can't it?

scottfines · 2026-03-20T16:39:15Z

        // map overload
-        testAddProperty(new HashMap<String, String>(){{put("k1", "v1");}},
-                        new HashMap<String, String>(){{put("k2", "v2");}},
+        testAddProperty(new HashMap<String, String>()


So while Java technically allows this, it's a fairly unclear way of constructing maps. A more standard way would be

testAddProperty(Map.of("k1", "v1"),Map.of("k2","v2"));

(using java.util.Map's static factory methods)

scottfines · 2026-03-20T16:45:16Z

-                logger.warn("Ignoring obsolete property {}", name);
+            {
+                long now = System.currentTimeMillis();
+                Long lastLogged = OBSOLETE_PROPERTY_LOG_TIME.get(name);


The consequences of this isn't super high (extra log messages), but this isn't thread safe. The line of logic as written is read-modify-write(get(), do stuff, thenput). Because the entire block isn't atomic, it is possible for two messages to concurrently arrive, both read that the last log time was outside the allowed window, and then write a new log message. It's not the end of the world obviously, but if we can fix it we should.

A more thread-safe way to do this would be to write it as

Long lastLogged = OBSOLETE_PROPERTY_LOG_TIME.putIfAbsent(name, now); if(lastLogged == null || (now - lastLogged) >= OBSOLETE_PROPERTY_LOG_INTERVAL_MS) logger.warn("Ignoring obsolete property {}", name);

This ensures that the map modification is atomic, so we won't have extra-message problems

scottfines · 2026-03-20T16:46:18Z

    private static final Pattern POSITIVE_PATTERN = Pattern.compile("(1|true|yes)");
    private static final Pattern NEGATIVE_PATTERN = Pattern.compile("(0|false|no)");
-
+    private static final Map<String, Long> OBSOLETE_PROPERTY_LOG_TIME = new ConcurrentHashMap<>();


nit: the name is slightly unclear: perhaps OBSOLETE_PROPERTY_LAST_LOG_TIMES?

scottfines · 2026-03-20T16:49:43Z

+        logAppender.list.clear();
+
+        // Wait 100 ms and try again - should log
+        Thread.sleep(100);


This can be somewhat unreliable--there is actually no guarantee that Thread.sleep(100) will actually wait 100 milliseconds. Usually it will, but sometimes it might not, and if it returns early, this test will fail. The net result is that the test will be treated as flaky (which we definitely don't want :)).

You would either need to guarantee that you wait a full 100ms(by tracking the time spent), or alter your solution design to not rely on system timings.

MSpryszynski · 2026-03-23T13:35:20Z

@scottfines Addressed all comments besides Map creation -> I rollbacked reformat changes to avoid possible conflicts during C* rebases in the future.

scottfines

This looks pretty good--the main concern I had was with the race condition, and that's resolved nicely. I had one last minor comment to get your thoughts on, but otherwise this is looking good

scottfines · 2026-03-23T14:13:03Z

+        // Setup custom clock for testing
+        testClock = new TestClock();
+        clockField = PropertyDefinitions.class.getDeclaredField("clock");
+        clockField.setAccessible(true);


This works, but you could also just create a @VisibleForTesting constructor in PropertyDefinitions that accepts a clock as a param. You'd need to also have a no-args constructor which uses a default clock, but that would allow you to avoid the reflection on this field, and also let you make the Clock final

Suggestion applied

scottfines

this looks good to me--once you can wrangle the CI tool to recognize your tests, you should be good to go. Nice work!

scottfines · 2026-03-24T19:22:51Z

+        logAppender.start();
+        logger.addAppender(logAppender);
+
+        lastLoggedTimeField = PropertyDefinitions.class.getDeclaredField("OBSOLETE_PROPERTY_LAST_LOG_TIMES");


I don't think you need this any longer, since you are using constructors to get the same level of access as before.

This field needs to be static, so creating it by constructor is not an option I think

…configuring threshold for repetition per property.

…configuring threshold for repetition per property. * Handled race condition scenario properly

…configuring threshold for repetition per property. * Created testing purpose constructor instead of using reflection in test

sonarqubecloud · 2026-04-07T10:44:57Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

cassci-bot · 2026-04-07T10:51:42Z

❌ Build ds-cassandra-pr-gate/PR-2229 rejected by Butler

4 regressions found
See build details here

Found 4 new test failures

Test	Explanation	Runs	Upstream
o.a.c.index.sai.cql.VectorCompaction100dTest.testOneToManyCompaction[version=ec enableNVQ=true]	REGRESSION	🔴⚪	0 / 30
o.a.c.index.sai.cql.VectorKeyRestrictedOnPartitionTest.partitionRestrictedWidePartitionBqCompressedTest[version=ca enableNVQ=false enableFused=false]	REGRESSION	🔴⚪	0 / 30
o.a.c.index.sai.cql.VectorSiftSmallTest.testSiftSmall[version=fa enableNVQ=false enableFused=false]	REGRESSION	🔴🔴	0 / 30
o.a.c.index.sai.cql.VectorTypeTest.testLimitLessThanInsertedRowCount[version=ca enableNVQ=false enableFused=false]	REGRESSION	🔴⚪	0 / 30

Found 8 known test failures

MSpryszynski force-pushed the cndb-13303 branch from 20c5812 to 5a2f70f Compare February 12, 2026 17:43

MSpryszynski force-pushed the cndb-13303 branch from 5a2f70f to 20cf772 Compare February 24, 2026 18:46

MSpryszynski force-pushed the cndb-13303 branch from 20cf772 to 6a03437 Compare March 11, 2026 13:52

scottfines requested changes Mar 20, 2026

View reviewed changes

MSpryszynski force-pushed the cndb-13303 branch from 6a03437 to 4bffb6c Compare March 23, 2026 12:05

scottfines reviewed Mar 23, 2026

View reviewed changes

MSpryszynski force-pushed the cndb-13303 branch from 4bffb6c to 52af837 Compare March 24, 2026 14:39

scottfines approved these changes Mar 24, 2026

View reviewed changes

MSpryszynski force-pushed the cndb-13303 branch from 81a3c2c to 52af837 Compare April 1, 2026 08:24

MSpryszynski added 7 commits April 7, 2026 11:34

CNDB-13303: Made warn log regarding obsolete property less spammy by …

64e9015

…configuring threshold for repetition per property.

CNDB-13303: Made warn log regarding obsolete property less spammy by …

85d7ebc

…configuring threshold for repetition per property. * Handled race condition scenario properly

CNDB-13303: Made warn log regarding obsolete property less spammy by …

45554b2

…configuring threshold for repetition per property. * Created testing purpose constructor instead of using reflection in test

Trigger pipeline

25e11a7

Trigger pipeline

f732b7a

Trigger pipeline

03fea04

Rollback Jenkinsfile change

b513c25

MSpryszynski force-pushed the cndb-13303 branch from 70c19fc to b513c25 Compare April 7, 2026 09:40

MSpryszynski merged commit e4a8489 into main Apr 7, 2026
2 of 4 checks passed

MSpryszynski deleted the cndb-13303 branch April 7, 2026 11:47

Conversation

MSpryszynski commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the issue

What does this PR fix and why was it fixed

Uh oh!

github-actions bot commented Feb 12, 2026 • edited by MSpryszynski Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist before you submit for review

Uh oh!

scottfines left a comment

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!

Choose a reason for hiding this comment

Uh oh!

MSpryszynski commented Mar 23, 2026

Uh oh!

scottfines left a comment

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!

scottfines left a comment

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!

sonarqubecloud bot commented Apr 7, 2026

Quality Gate passed

Uh oh!

cassci-bot commented Apr 7, 2026

❌ Build ds-cassandra-pr-gate/PR-2229 rejected by Butler

Found 4 new test failures

Found 8 known test failures

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MSpryszynski commented Feb 12, 2026 •

edited

Loading

github-actions bot commented Feb 12, 2026 •

edited by MSpryszynski

Loading